Skip to content

Conversation

@george42-ctds
Copy link
Contributor

Jira Ticket: None

New Features

Breaking Changes

Bug Fixes

  • Handle resource_paths without content after a forward slash ('/')

Improvements

Dependency updates

Deployment changes

@github-actions
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@george42-ctds george42-ctds changed the title (fix): Handle request_paths without content after slash ('/') (fix): Handle resource_paths without content after slash ('/') Jul 26, 2022
Comment on lines +116 to +119
for p in data["resource_paths"]:
if not p.split("/")[1:]:
msg = f"Request creation failed. Resource path '{p}' does not have content after a forward slash ('/')."
raise_error(logger, msg, body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the validity of resource paths should be Arborist's concern, not Requestor's. It looks like Requestor would work fine no matter the format of the resource path. What error/issue are we actually trying to fix here?

Copy link
Contributor Author

@george42-ctds george42-ctds Jul 29, 2022

Choose a reason for hiding this comment

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

Requestor returns a 500 when receiving a request with a bad resource_path (without a slash), after receiving a 400 from Arborist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is when creating a new request, it sounds to me like we should catch that 400 from arborist, and return a 400 to the user after getting one from arborist. We shouldn't duplicate the input validation from the server into the client. Arborist probably returns a nice error message that Requestor can forward to the user

if the error happens later, during the automatic call to arborist when requests are approved, then it's a bit more tricky

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.

5 participants