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: Add migration for workspaceName constraints #5202

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Aug 15, 2024

This migration rewrites all events containing a workspaceName, which doesn't match the new workspaceName constraints and rewrites them with (shortend) a md5 hash of themself.

Run the migration with:

./flow migrateevents:migratepayloadtovalidworkspacenames

If the migration found some workspaceNames or baseWorkspaceNames to migrate, please run a projection replay of the workspace projection.

./flow cr:projectionreplay --projection workspace

This sould just be neccesary if you update your Neos 9 from <= beta10 to a newer version.

Fixes #5201

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. Thanks a lot for cleaning after me 🫣

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Migration was not necessary.

as desired :)

@kitsunet kitsunet merged commit 380a945 into neos:9.0 Aug 15, 2024
11 checks passed
@dlubitz dlubitz deleted the task/5201-workspacename-migration branch August 15, 2024 11:05
@mhsdesign
Copy link
Member

Hä but no,

Invalid workspace name "Test-wy99x" given.

i dont understand? It didnt work after all?

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

🤔 The migration just handles to long workspaceNames

@kitsunet
Copy link
Member

Wait what is the problem with that? uppercase?

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

Yes, the new constraint only allows lowercase.

    private const PATTERN = '/^[a-z][a-z0-9\-]{0,' . (self::MAX_LENGTH - 1) . '}$/';

@mhsdesign
Copy link
Member

jup bastis change only allows lowercase workspace names... i guess Test-wy99x should be transformed to test-wy99x right? Or a hash as well? live and user-admin are safe btw

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

I'd go for lowercase everything.

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

🙈 Also, it's not allowed to start with a number anymore ... and the md5 hash could be a number.

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

Wait.... but the regex in the migration should find your Test-wy99x ... but mysql Regex is case insensitive

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

Choose a reason for hiding this comment

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

I read through this more in-depth now and I wonder why you used SUBSTR(..., 1, ...) – if the original value was uppercase it would still be invalid afterwards.
My original suggestion was:

CONCAT('w', MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')))))

(note the "w"-prefix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The md5 hash is 32 chars long, but the workspaceName can just be 30 chars long. So I had to shorten it. To reduce the chance of collision, I removed the prefix to be able to keep 30 chars of the hash instead of 29.

But yeah...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, sorry

LEFT(CONCAT('w', MD5(JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName'))))), 30)

could work

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I hadn't realized: SUBSTR(..., 1, x) is the same as LEFT(..., x) (since it's not zero-based like in PHP).
Still, the first character of an MD5 could be a digit

@dlubitz
Copy link
Contributor Author

dlubitz commented Aug 15, 2024

Bääähhh... what about "baseWorkspaceName"? This also needs to be migrated, right? @bwaidelich

And after the migration a projection replay is needed for workspace projection.

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