-
Notifications
You must be signed in to change notification settings - Fork 3
fix: batch creation creates correct metadata #1000
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
Conversation
…atch modification tests This commit modifies the SQL migration to correct the metadata key for read-only keys, ensuring consistency in the handling of 'readonly_key' and 'stream_owner'. Additionally, it introduces a new test to verify that read-only metadata cannot be modified in batch operations, enhancing the robustness of the metadata management system.
This commit refactors the `create_stream` action to delegate the stream creation process to the `create_streams` batch implementation, enhancing consistency and simplifying the code. The previous permission checks and validations have been removed from `create_stream`, streamlining the action's logic.
Bug Report Checklist
@outerlook, please use git blame and specify the link to the commit link that has introduced this bug.
|
This commit modifies the error message assertion in the `TestStreamIDValidation` test to check for "duplicate key value violates" instead of "already exists". This change ensures that the error message accurately reflects the underlying database constraint violation when attempting to create a duplicate stream ID for the same owner.
Time Submission Status
|
@outerlook what's wrong with already deployed streams? How much impacted? |
Every that was deployed using batch create until now. Single and batch create streams diverged and the created metakeys weren't correct. Before this patch, people could just manually alter their own stream owners (not others') and their own This offers no advantage to them; it could only create bad states for themselves if they manually tried. It is highly unlikely that they did. the patch was already executed on server to fix this, which is only: UPDATE metadata
SET metadata_key = 'readonly_key'
WHERE metadata_key IN (
'readonly_key:stream_owner',
'readonly_key:readonly_key'
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops the changes broke the CI
Will see if I can get a quick fix
The `create_streams` action was generating duplicate `row_id` values for metadata entries when multiple streams were created in quick succession within the same test context. This was due to the UUID v5 generation relying only on a base UUID derived from `@txid` and a sequential row number, which could lead to collisions across different stream creations if `@txid` was similar and the metadata keys generated in the same order. This commit modifies the `row_id` generation in `create_streams` to include the `stream_id` and `metadata_key` in the "name" portion of the UUID v5, ensuring a unique `row_id` for each distinct metadata entry across all streams created. This resolves the "duplicate key value violates unique constraint `metadata_pkey`" error observed in tests like `TestAUTH04_NestedComposePermissions`.
The `create_streams` action was generating duplicate `row_id` values for metadata entries when multiple streams were created in quick succession within the same test context. This was due to the UUID v5 generation relying only on a base UUID derived from `@txid` and a sequential row number, which could lead to collisions across different stream creations if `@txid` was similar and the metadata keys generated in the same order. This commit modifies the `row_id` generation in `create_streams` to include the `stream_id` and `metadata_key` in the "name" portion of the UUID v5, ensuring a unique `row_id` for each distinct metadata entry across all streams created. This resolves the "duplicate key value violates unique constraint `metadata_pkey`" error observed in tests like `TestAUTH04_NestedComposePermissions`. <!--- Provide a general summary of your changes in the Title above by following our Developer Guidelines --> ## Description <!--- Describe your changes in detail; use bullet points. --> ## Related Problem <!--- If this pull request relates to an existing Problem, please link to it here (https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) --> <!-- example: resolves: #112330 --> resolves: #1000 (review) ## How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. -->
Description
Related Problem
How Has This Been Tested?
note: I'll need to execute a fix on our server directly for already created streams