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

First version #1

Merged
merged 22 commits into from
Nov 12, 2024
Merged

First version #1

merged 22 commits into from
Nov 12, 2024

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Oct 10, 2024

@pbrisbin pbrisbin changed the title pb/first First version Oct 10, 2024
src/Network/Wai/Middleware/OpenApi.hs Outdated Show resolved Hide resolved
src/Network/Wai/Middleware/OpenApi/Validate.hs Outdated Show resolved Hide resolved
src/Network/Wai/Middleware/OpenApi/Validate.hs Outdated Show resolved Hide resolved
src/Network/Wai/Middleware/OpenApi.hs Outdated Show resolved Hide resolved
tests/TestApp.hs Show resolved Hide resolved
Comment on lines +40 to +44
errorResponse :: Status -> Text -> NonEmpty Text -> Response
errorResponse status msg =
responseLBS status [(hContentType, "application/json")]
. encode
. Error msg

Choose a reason for hiding this comment

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

Is it sort of ironic that this response probably violates your openapi spec?

Copy link
Member Author

@pbrisbin pbrisbin Oct 10, 2024

Choose a reason for hiding this comment

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

My original plan was to derive ToSchema Error and export it, then users of this library could add it to their own OpenApi. This would make our own responses valid according to ourselves.

I think I can still do that, but I realized that users might already have 400 responses specified throughout, so I can't exactly picture yet how to incorporate this schema. I ended up implicitly leaning on the "well then drop down to validate and change respondError if this matters" as the solution.

pbrisbin and others added 2 commits October 10, 2024 14:24
Co-authored-by: Chris Martin <ch.martin@gmail.com>
- Remove Settings type
- Remove ValidationErrors type
- Remove ValidateOption internals
- Introduce ValidateT
- Separate request vs response errors
- Separate schema-not-found, etc as errors
- Document higher and lower-level usage
@chris-martin chris-martin mentioned this pull request Oct 15, 2024
@github-actions github-actions bot mentioned this pull request Oct 15, 2024
@pbrisbin pbrisbin marked this pull request as ready for review November 11, 2024 19:33
@pbrisbin
Copy link
Member Author

pbrisbin commented Nov 12, 2024

@chris-martin I think this is ready to merge, if not release. Check out the PathMapSpec to see how it behaves. The implementation is naive, but I think its performance scales with the number of distinct paths in the spec, which I'd expect to be manageable in even very complicated of APIs. I suspect the performance of validation overall is dominated by looking up the schema once a path is found. Still, we can always benchmark and improve later.

@chris-martin
Copy link

Starting with the straightforward linear search approach seems like a wise way to go.

@pbrisbin pbrisbin merged commit b6167ac into main Nov 12, 2024
5 checks passed
@pbrisbin pbrisbin deleted the pb/first branch November 12, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants