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

Add errors fast api #18093

Merged
merged 17 commits into from
Jul 15, 2024
Merged

Add errors fast api #18093

merged 17 commits into from
Jul 15, 2024

Conversation

arash77
Copy link
Collaborator

@arash77 arash77 commented May 3, 2024

What did you do?

  • Change 422 Validation error response
  • Create MessageExceptionModel for the schema of error responses

Why did you make this change?

The Validation error should be replaced by the Bad request error as it is not the real model that is returning.

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 24.1 milestone May 3, 2024
@mvdbeek mvdbeek modified the milestones: 24.1, 24.2 May 14, 2024
code_int = int(str(code)[:3])
if code_int in [0, 204] or code_int in responses:
continue
responses[code_int] = {
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if the literals "4XX" and "5XX" work (https://swagger.io/docs/specification/describing-responses/) ? I think that would be a bit less verbose

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean using 4XX and 5XX instead of all the code errors in docs?
It returns this error while updating client API schema: SyntaxError: An identifier or keyword cannot immediately follow a numeric literal.

Copy link
Member

Choose a reason for hiding this comment

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

I meant the string "4XX" and "5XX", as listed in the swagger docs

Copy link
Contributor

@davelopez davelopez Jul 9, 2024

Choose a reason for hiding this comment

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

It returns this error while updating client API schema: SyntaxError: An identifier or keyword cannot immediately follow a numeric literal.

I think this is a bug in the version of openapi-typescript we are using:

https://github.com/galaxyproject/galaxy/actions/runs/9853821142/job/27205208465?pr=18093#step:10:189

Updating it to version 6.7.6 generates the correct typescript schema, but it also changes (probably fixes) some of the types that were generated.

For example:
Record<string, never> -> unknown which I think makes more sense and they probably fixed it.

I can try to open a new PR for bumping the openapi-typescript version and fixing the new type issues in the client.

Copy link
Member

Choose a reason for hiding this comment

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

@davelopez I can pop just that upgrade out of https://github.com/dannon/galaxy/tree/openapi-typescript-fetch-upgrade if we want, following up on the fetcher swapover later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed this earlier 😅
I have a PR just bumping the version to 6.7.6 and fixing the issues in the client. But I agree, we should follow up and try to swap the fetcher later.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, sounds good!

@arash77 arash77 force-pushed the add_errors_fast_api branch from 16ec128 to ba3a24d Compare July 11, 2024 12:24
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot!

@davelopez
Copy link
Contributor

Thank you @arash77!!

Test failures are unrelated.

@davelopez davelopez merged commit e6bb795 into galaxyproject:dev Jul 15, 2024
52 of 54 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@arash77 arash77 deleted the add_errors_fast_api branch July 15, 2024 12:50
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.

4 participants