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

fix: validate workspace name #5647

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Dec 22, 2022

Description

WEB-746

Test Plan

  • Go to workspace
  • Check if Creating/Editing a workspace with an all whitespaced name like on webUI modal causes validation errors
  • Check if Creating/Editing a workspace with an all whitespaced name though API causes errors

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@netlify
Copy link

netlify bot commented Dec 22, 2022

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 565aa53
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/63b5fb0091d194000862e9bb

@keita-determined keita-determined force-pushed the fix/validate-workspace-name branch 4 times, most recently from 8cc2d3b to bb0e7e6 Compare December 28, 2022 19:17
@@ -281,6 +281,10 @@ func (a *apiServer) PostWorkspace(
return nil, status.Errorf(codes.InvalidArgument,
"name '%s' must be at most 80 character long", req.Name)
}
if len(strings.Trim(req.Name, " ")) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we really want to restrict workspace names this way then we should be accounting for all whitespace characters. There is a unicode utility to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

probably something like this should trim both common whitespace and unicode separators:

var whitespaceText = regexp.MustCompile("[\\pZ[:space:]]")

func main() {
	s := "Hello, p\tl\naygr ou\u2028n\u00a0d"
	fmt.Println(whitespaceText.ReplaceAllLiteralString(s, ""))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we just wanna trim leading and trailing whitespaces to accept names like Hello World or test v1, so i made it like this

len(strings.TrimFunc(name, unicode.IsSpace)) == 0

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, that should work. I did not realize we want to trim these spaces (what if user really wants to have them there). if we do, perhaps we'd want to do the same every time we parse workspace names in other APIs, e.g. in the experiment config on experiment submission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see. then beckend just checks if the name isnt whitespace-only, and dont trim whitespace if there are at least 1 non-whitespace.

Copy link
Contributor

@gt2345 gt2345 left a comment

Choose a reason for hiding this comment

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

Works as described, but do make sure the exact rules we want to follow before merging.

Copy link
Contributor

@julian-determined-ai julian-determined-ai left a comment

Choose a reason for hiding this comment

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

Left minor formatting suggestions but otherwise great work!

@keita-determined keita-determined merged commit 99ba058 into determined-ai:master Jan 4, 2023
@keita-determined keita-determined deleted the fix/validate-workspace-name branch January 4, 2023 23:08
@dannysauer dannysauer added this to the 0.19.10 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants