-
Notifications
You must be signed in to change notification settings - Fork 67
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
fix: Fixes issues with special characters in object name #2156
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=a8044212762b171870e48cf5416740397dcd97d1 |
weave/weave_client.py
Outdated
# Replaces any non-alphanumeric characters with a single dash and removes | ||
# any leading or trailing dashes. This is more restrictive than the DB | ||
# constraints and can be relaxed if needed. | ||
res = re.sub(r"[._-]+", "-", re.sub(r"[^\w._]+", "-", name)).strip("-_") |
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.
i am wondering if this will result in any changes to integration names?
@@ -57,7 +58,20 @@ def wb_run_id_validator(s: typing.Optional[str]) -> typing.Optional[str]: | |||
return s | |||
|
|||
|
|||
def _validate_object_name_charset(name: str) -> None: | |||
# Object names must be alphanumeric with dashes | |||
invalid_chars = re.findall(r"[^\w._-]", name) |
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.
This is quite restrictive - could frustrate people?
@@ -34,7 +34,7 @@ describe('parseRef', () => { | |||
}); | |||
it('parses a ref with spaces in entity', () => { | |||
const parsed = parseRef( | |||
'weave:///Entity%20Name/project/object/artifact-name:artifactversion' | |||
'weave:///Entity Name/project/object/artifact-name:artifactversion' |
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.
This was not a valid ref - in fact it was a bug in ref creation from Browse3
@@ -102,20 +102,6 @@ describe('parseRef', () => { | |||
weaveKind: 'object', | |||
}); | |||
}); | |||
it('parses a ref with escaped spaces in name and projectName', () => { |
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.
Same here, not a valid ref
Attaching this ticket from triage to this pr, curly braces should be supported. |
This PR should sanitize these away |
Simplified version of #2145 that takes a different approach to fixing the problem by implementing constraints on the names rather than url encoding.
Allowed characters:
\w._-
This PR does 3 things:
Browse3
to use decoded params from the URL, which avoids creating invalid refs on UI side.The effective result of this is:
Deployment can be an any order, hence a common PR to centralize all these changes