Allow dollar in layer name #8241
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to the validation rules for dataset names in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Can we allow it for layers but not for datasets? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/admin/dataset/dataset_components.tsx (1)
44-44: Consider using a named constant for the validation patternSince this pattern is used in multiple places (layer names and dataset names) and needs to stay in sync with backend validation, consider extracting it into a named constant.
+const VALID_NAME_PATTERN = /^[0-9a-zA-Z_.\-$.]+$/; +const VALID_NAME_MESSAGE = "Only letters, digits and the following characters are allowed: . _ - $"; export const layerNameRules = [ { min: 1, }, // Note that these rules are also checked by the backend { - pattern: /^[0-9a-zA-Z_.\-$.]+$/, - message: "Only letters, digits and the following characters are allowed: . _ - $", + pattern: VALID_NAME_PATTERN, + message: VALID_NAME_MESSAGE, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/javascripts/admin/dataset/dataset_components.tsx(1 hunks)
🔇 Additional comments (2)
frontend/javascripts/admin/dataset/dataset_components.tsx (2)
44-45: LGTM with verification needed!
The changes correctly implement the dollar sign support in the validation regex and update the corresponding error message. However, since the comment indicates these rules are also checked by the backend, we should verify the backend validation alignment.
Let's verify the backend validation rules:
44-45: Verify UI components handling layer/dataset names
Since this change affects both layer and dataset name validation, we should verify that all UI components properly handle and display names containing dollar signs.
✅ Verification successful
Based on the search results, I can now generate a final response since we have enough information about how layer/dataset names are handled in the UI components:
UI components properly handle special characters in names
The validation pattern change for allowing dollar signs ($) in layer/dataset names is safe to implement. The codebase shows that:
- Names are rendered directly in UI components without any special escaping/formatting that would break with dollar signs
- Name validation is only enforced at the input level in dataset_components.tsx
- No hardcoded assumptions about valid name characters were found in display/rendering logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for components that handle layer or dataset names
# Find components that might display or manipulate layer/dataset names
rg -t typescript -t tsx "layer.*name|dataset.*name" --glob "!*test*" -l
# Look for string escaping or formatting of names
rg -t typescript -t tsx "toString|format.*name|display.*name" --glob "!*test*"
# Check for any hardcoded assumptions about valid characters
rg -t typescript -t tsx "[a-zA-Z0-9_\.-]+" --glob "!*test*"
Length of output: 321
Script:
#!/bin/bash
# Look for components that handle layer or dataset names using ripgrep
# Note: Using just 'rg' without typescript/tsx file type flags
# Find components that might display or manipulate layer/dataset names
rg "layer.*name|dataset.*name" --glob "*.ts*" --glob "!*test*" -l
# Look for string escaping or formatting of names
rg "toString|format.*name|display.*name" --glob "*.ts*" --glob "!*test*"
# Check for any hardcoded assumptions about valid characters in validation patterns
rg "[a-zA-Z0-9_\\.-]+" --glob "*.ts*" --glob "!*test*"
# Additionally, let's check for any UI components that might be rendering these names
rg "<.*?(layer|dataset).*?name.*?>" --glob "*.tsx" --glob "!*test*" -A 2
Length of output: 146673
Script:
#!/bin/bash
# Let's search more specifically for UI components that render layer/dataset names
# and check for any string formatting or display logic
# Look for components that render names in JSX
rg -l "name=|name:|name =" --glob "*.tsx" --glob "!*test*"
# Look for name-related props and types
rg "name: string|name\?: string|name: \{|name\?: \{" --glob "*.ts*" --glob "!*test*"
# Look for specific name validation or formatting functions
rg "validate.*name|format.*name|display.*name" --glob "*.ts*" --glob "!*test*"
# Look for any dollar sign related validation or escaping
rg "\$|\\$" --glob "*.ts*" --glob "!*test*" -A 1
Length of output: 164537
…ow-dollar-in-layer-names
MichaelBuessemeyer
changed the title
| @@ -17,6 +17,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released | |||
| ### Changed | |||
| - Renamed "resolution" to "magnification" in more places within the codebase, including local variables. [#8168](https://github.com/scalableminds/webknossos/pull/8168) | |||
| - Reading image files on datastore filesystem is now done asynchronously. [#8126](https://github.com/scalableminds/webknossos/pull/8126) | |||
| - Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the changelog entry more precise.
The current entry could be misinterpreted to suggest that $ is allowed in all names. Consider revising it to explicitly state the distinction between layer names and dataset names.
-Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241)
+Layer names (but not dataset names) are now allowed to contain the `$` character. [#8241](https://github.com/scalableminds/webknossos/pull/8241)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241) | |
| - Layer names (but not dataset names) are now allowed to contain the `$` character. [#8241](https://github.com/scalableminds/webknossos/pull/8241) |
There was a problem hiding this comment.
The functionality you describe works as intended, but I noticed two things during testing:
- When trying to rename a volume tracing layer from within an annotation, $ is still not allowed (
This layer name includes the disallowed character "$". Please remove it to set the layer name.) - When changing the layer name in the advanced datasource settings, the first click always triggers a warning toast, indicating that one of the fields is not valid (
Please check that all form fields are valid.), but no field is highlighted. This happens regardless of whether a $ character is included or not and only on the first submission. Clicking submit a second time succeeds. I briefly tried to debug this, but didn't find the cause. This issue probably existed before, so can also be solved in a separate PR.
…ow-dollar-in-layer-names
|
Thanks for the review @daniel-wer 🙏
Is a known bug in the frontend code: See: #8172
Yeah, the frontend resticts this. While trying to add Moreover, just adjusting the fronend is not enough. The backend also has a check in place in the DB. Therefore, a migration would also be necessary here 🥴 (quite the amount of work for such a small change) |
|
Both dataset and annotation layer names are used in the URIs for zarr streaming. This should be tested with
Maybe it can be done in two steps? First the chanage for dataset layers as implemented here, and then as a follow-up also for annotation layer names? As I understand it, the current use case is about color layers only (predictions from voxelytics), so no volume annotation layers with these layers fallback segmentations should be created for the moment. This should probably also be tested in the second stage then. |
Just doing one pr is fine for me. It's was just a little unexpected to me that this small change requires a migration. Thanks for the hint, I'll do the zarr streaming testing :D |
…ow-dollar-in-layer-names
Works with color layer, segmentation layer, volume layer with fallback layer, and without fallback layer. All with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
conf/evolutions/reversions/125-allow-dollar-in-layer-names.sql (1)
6-6: Consider batching the update operationThe current update might impact performance on large tables. Consider batching the operation for better performance and reduced lock time.
DO $$ DECLARE batch_size INTEGER := 1000; affected_rows INTEGER; BEGIN LOOP UPDATE webknossos.annotation_layers SET name = regexp_replace(name, '\$', '', 'g') WHERE name ~* '\$' LIMIT batch_size; GET DIAGNOSTICS affected_rows = ROW_COUNT; EXIT WHEN affected_rows = 0; COMMIT; END LOOP; END $$;frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx (2)
51-51: Update variable name and add documentation about URI safetyThe variable name
uriSafeCharactersRegexmight be misleading since$is not typically considered URI-safe. Consider documenting the implications or renaming the variable.- const uriSafeCharactersRegex = /[0-9a-zA-Z-._$]+/g; + // Note: The $ character is allowed in layer names but requires proper URI encoding in URLs + const allowedLayerCharactersRegex = /[0-9a-zA-Z-._$]+/g;
Line range hint
51-63: Update error messages to reflect allowed charactersThe error messages should be updated to accurately reflect that the
$character is now allowed in layer names.Consider updating the relevant message in your messages file:
"tracing.volume_layer_name_includes_invalid_characters": (chars: string) => `Layer name contains invalid characters: ${chars}. Only alphanumeric characters, hyphens, dots, underscores, and $ are allowed.`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)conf/evolutions/125-allow-dollar-in-layer-names.sql(1 hunks)conf/evolutions/reversions/125-allow-dollar-in-layer-names.sql(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx(1 hunks)tools/postgres/schema.sql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (4)
conf/evolutions/125-allow-dollar-in-layer-names.sql (1)
6-7: Consider validating existing data before constraint change
While the new constraint correctly allows the $ character, it would be safer to first verify that no existing layer names contain other invalid characters that might violate the new constraint.
Consider adding a data validation step before the constraint change:
DO $$
BEGIN
IF EXISTS (
SELECT name FROM webknossos.annotation_layers
WHERE name !~* '^[A-Za-z0-9\-_\.\$]+$'
) THEN
RAISE EXCEPTION 'Found layer names that would violate the new constraint';
END IF;
END $$;frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx (1)
Line range hint 51-63: Verify URI handling in the application
Since we're allowing $ in layer names, we need to ensure proper URI encoding/decoding throughout the application.
✅ Verification successful
Let me gather more information about how layer names are used in URLs and API endpoints.
Based on the gathered information, I can now provide the final response:
The $ character in layer names is properly handled throughout the application
The codebase shows robust URI handling:
- URL Manager (
url_manager.ts) explicitly handles URI encoding/decoding with documented character sets - Layer names in URLs are used safely in API endpoints with proper encoding
- There's a database-level constraint ensuring URL-safe layer names
- The validation regex
/[0-9a-zA-Z-._$]+/aligns with the application's URL handling capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for URI encoding/decoding handling in the codebase
# Expected: Find relevant URI handling code to verify proper encoding
echo "Checking for URI encoding/decoding usage..."
rg -A 2 "encodeURI(Component)?"
echo "Checking for URL construction with layer names..."
ast-grep --pattern 'url = $_ + $layer.name + $_'
Length of output: 2676
Script:
#!/bin/bash
# Check how layer names are used in URLs and API endpoints
echo "Checking for layer name usage in URLs and API endpoints..."
rg -A 3 "layer.*name.*url|url.*layer.*name"
echo "Checking volume layer related API endpoints..."
rg -A 3 "volume.*layer.*api|api.*volume.*layer"
echo "Checking for layer name validation patterns..."
rg -A 3 "layer.*name.*valid|valid.*layer.*name"
Length of output: 10021
tools/postgres/schema.sql (2)
23-23: LGTM: Schema version increment is appropriate.
The schema version bump from 124 to 125 aligns with the database changes for allowing dollar signs in layer names.
59-59: LGTM: Layer name constraint correctly allows dollar signs.
The CHECK constraint's regex pattern has been updated to allow dollar signs ($) in layer names while maintaining other character restrictions. This implements the requirement at the database level.
Let's verify that dataset names still prohibit dollar signs:
✅ Verification successful
Dataset name constraints do not allow dollar signs, maintaining security.
Based on the search results, I can confirm that dataset names have different constraints than layer names. Looking at the schema in tools/postgres/schema.sql, datasets are created in the webknossos.datasets table, and the name constraints for datasets are defined in webknossos.dataStores table which explicitly shows:
name VARCHAR(256) PRIMARY KEY NOT NULL CHECK (name ~* '^[A-Za-z0-9\-_\.]+$')This pattern does not include the $ character, confirming that dataset names still prohibit dollar signs while only layer names allow them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dataset name constraints still prohibit dollar signs
# Test: Search for dataset table constraints. Expect: No $ in the name constraint pattern
rg -A 5 'datasets.*name.*CHECK'
Length of output: 33
Script:
#!/bin/bash
# Let's try a broader search for dataset table definition and constraints
rg -i "create table.*datasets?" -A 10
Length of output: 13164
Script:
#!/bin/bash
# Alternative search looking for name constraints in schema.sql
cat tools/postgres/schema.sql | grep -A 5 "datasets.*name"
Length of output: 60
Script:
#!/bin/bash
# Search for any name constraints in schema.sql
cat tools/postgres/schema.sql | grep -A 2 "name.*CHECK"
Length of output: 1333
| @@ -48,7 +48,7 @@ export function checkLayerNameForInvalidCharacters(readableLayerName: string): V | |||
| message: messages["tracing.volume_layer_name_starts_with_dot"], | |||
| }; | |||
| } | |||
| const uriSafeCharactersRegex = /[0-9a-zA-Z-._]+/g; | |||
| const uriSafeCharactersRegex = /[0-9a-zA-Z-._$]+/g; | |||
There was a problem hiding this comment.
If I remember correctly you found that these characters don't necessarily have to be url safe, right? If so, maybe rename to validLayerNameCharactersRegex or so instead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
342-353: Consider using a longer timeout for validation deferral.The setTimeout implementation correctly defers the validation to handle antd's asynchronous validation. However, a 1ms timeout might be too short on slower devices.
- 1, + 50,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(1 hunks)
🔇 Additional comments (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
401-403: LGTM! Improved layer name validation.
The validation now correctly uses form.getFieldValue to check layer name uniqueness against the current form state, which is more reliable than direct array filtering.
401-403: Verify the distinction between layer and dataset name validation.
The PR objective states that dollar signs should be allowed in layer names but disallowed in dataset names. Let's verify this implementation.
✅ Verification successful
Distinction between layer and dataset name validation is correctly implemented
The verification shows that the distinction is properly implemented:
- Layer names allow dollar signs:
/^[0-9a-zA-Z_.\-$.]+$/ - Dataset names are more restrictive:
/^[0-9a-zA-Z_.-]+$/
The implementation in dataset_components.tsx correctly enforces:
- Dollar signs ($) are allowed in layer names
- Dollar signs are disallowed in dataset names
- Both validations share common rules but dataset names have additional restrictions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the validation rules for layer and dataset names
# Search for layer name validation rules
echo "=== Layer Name Rules ==="
rg "layerNameRules.*=.*" -A 5
# Search for dataset name validation rules
echo "=== Dataset Name Rules ==="
rg "getDatasetNameRules.*=.*" -A 5
Length of output: 1290
Script:
#!/bin/bash
# Let's check the shared rules and dataset name pattern validation
echo "=== Shared Rules ==="
rg "sharedRules.*=.*" -A 5
echo "=== Dataset Name Pattern ==="
rg "pattern.*dataset.*name" -i -A 2
Length of output: 609
Script:
#!/bin/bash
# Let's check the complete dataset validation implementation
echo "=== Dataset Name Validation ==="
rg "getDatasetNameRules" -A 20
# Let's also check for any other dataset name pattern validations
echo "=== Dataset Pattern Validations ==="
ast-grep --pattern 'pattern: /[^}]+dataset[^}]+/'
Length of output: 8309
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
$incheckLayerNameForInvalidCharacters$for datasetNamesIssues:
(Please delete unneeded items, merge only when none are left open)