Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Case sensitive migration and migration of baseWorkspaceName #5205

Merged
merged 6 commits into from
Aug 17, 2024

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Aug 15, 2024

Follow up of #5202

  • Increase workspaceName lenght to 36 chars.
  • Lower constraints on content so also a hash or UUID is valid.
  • Ensure migration works case sensitive.
  • Ensure newly created user-workspaceNames will be valid.
  • Also applying to baseWorkspaceName.

Fixes #5201

@dlubitz dlubitz self-assigned this Aug 15, 2024
…ceName with a leading letter. Also applying to baseWorkspaceName.
@dlubitz dlubitz force-pushed the task/5201-workspacename-migration branch from d43826e to 4ef9987 Compare August 15, 2024 11:48
@mhsdesign
Copy link
Member

mhsdesign commented Aug 15, 2024

Also applying to baseWorkspaceName.

ohhhh dammm yes i feel we are just getting started with the can of worms. Senf or mayo?

Just food for thought: Is it realistic that there will be one workspace foo already and another Foo and this will clash em? I guess not and i think we can ignore this case and for the case someone really has this problem help them via manual replace :D

…ceName with a leading letter. Also applying to baseWorkspaceName.
@mhsdesign
Copy link
Member

At first i thought it was really clever to do it via sql but maybe as its just a onetime thing we could go the manual php loop round? The thing is its getting more and more complex. I wouldnt mind this taking a little longer ^^ Though i will also approve this and i admire that you can write sql like that :D

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

Should be fine now(TM) . We would also cover uppercase/lowercase by the md5 now.

UPDATE {$eventTableName}
SET
payload = JSON_SET(payload, '$.workspaceName', SUBSTR(MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName'))), 1, 30))
payload = JSON_SET(payload, '$.workspaceName', LEFT(CONCAT('w', MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')))), 30))
Copy link
Member

@bwaidelich bwaidelich Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this should work, if we change the WorkspaceName regex to ^[a-z0-9\-]{0,36}

Suggested change
payload = JSON_SET(payload, '$.workspaceName', LEFT(CONCAT('w', MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')))), 30))
payload = JSON_SET(
payload,
'$.workspaceName',
IF(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')) REGEXP '^[a-z0-9\-]{0,36}$',
LOWER(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName'))),
MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')))
)
)

@dlubitz dlubitz marked this pull request as draft August 16, 2024 10:41
@dlubitz dlubitz marked this pull request as ready for review August 16, 2024 13:20
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 by reading. Thank you! <3

@nezaniel nezaniel self-requested a review August 16, 2024 22:14
Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not seem to work.... after applying the event migration I still get "Invalid workspace name (user-Nezaniel) given." during cr:projectionreplayall

@nezaniel
Copy link
Member

nezaniel commented Aug 16, 2024

I found the issue; There are two more workspaceName proeprties (sourceWorkspaceName and targetWorkspaceName). I fixed it and it now works on my starship(tm)

@nezaniel nezaniel merged commit 48e5d0a into neos:9.0 Aug 17, 2024
9 checks passed
@mhsdesign
Copy link
Member

post merge +1 thanks (and also thanks for the note with having to replay the workspace module ^^)

@dlubitz dlubitz deleted the task/5201-workspacename-migration branch September 17, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

WorkspaceNames in legacy events may not meet new constraints
4 participants