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

[WIP] Open api3 rebased #928

Draft
wants to merge 55 commits into
base: master
Choose a base branch
from
Draft

Conversation

numbata
Copy link
Contributor

@numbata numbata commented May 8, 2024

This is rebased version of the branch oapi-3 from #744

Current state:

Finished in 11.31 seconds (files took 0.94256 seconds to load)
644 examples, 34 failures, 2 pending

@numbata numbata force-pushed the open-api3-rebased branch from 290b700 to 9b6cb2a Compare July 12, 2024 08:56
@dblock
Copy link
Member

dblock commented Jul 12, 2024

👀 Looking forward to this!

numbata added 2 commits July 13, 2024 10:07
…eaner

Add ApiClassDefinitionCleaner module to track and remove classes that inherit from
Grape::API. This change prevents classname collisions between different specs, ensures
that no remaining classes interfere with subsequent tests, and prevents ArgumentError
caused by unexpected module inclusions.

Details:

- Created ApiClassDefinitionCleaner module with before(:all) and after(:all) hooks.
- Hooks track initial and current state of Grape::API subclasses.
- Identifies and removes newly defined classes after each spec run to prevent
  unintentional changes and conflicts.
@numbata
Copy link
Contributor Author

numbata commented Jul 13, 2024

Finished in 7.49 seconds (files took 0.9935 seconds to load)
644 examples, 34 failures, 2 pending, 76 errors occurred outside of examples

@numbata
Copy link
Contributor Author

numbata commented Jul 13, 2024

Finished in 11.31 seconds (files took 0.94256 seconds to load)
644 examples, 34 failures, 2 pending

@numbata
Copy link
Contributor Author

numbata commented Jul 14, 2024

Finished in 11.22 seconds (files took 0.93428 seconds to load)
644 examples, 52 failures, 2 pending

The reason for 📈 🔴 is: after alignment of the openapi3 schema builder with the openapi2, the request object goes under the $ref, some specs become red.

@numbata
Copy link
Contributor Author

numbata commented Jul 14, 2024

Finished in 12.34 seconds (files took 1 second to load)
644 examples, 69 failures, 2 pending

📈 🔴 after the addition of validation against the OpenAPI3 specification.

@numbata
Copy link
Contributor Author

numbata commented Jul 15, 2024

My intermediate thoughts and Suggestions:

First of all I would like to say that @blakepettersson has done a fantastic job. I really appreciate the effort, especially the specs, which make me confident about achieving OpenAPI3 schema from Grape endpoints.

However, while working on the branch, I felt that the chosen strategy might not be ideal. Copying the existing OpenAPI2 code and modifying it for OpenAPI3 resulted in two separate codebases doing almost the same job, making synchronization difficult.

In addition, the new DryValidation and DrySchema support for request parameter definition is not currently supported in the existing code. This could be a deterrent for potential users who may choose other frameworks if Grape lacks this feature.

But there's hope. I believe that we should separate the process of gathering endpoint information from the process of schema generation, possibly by using intermediate structures. These structures can be fully populated first, and then we can generate the specific OpenAPI version schema.

One possible implementation is to use DrySchemas as schema objects that can be translated from native Grape params. This approach offers the benefits of schema composition, which is a valuable feature in OpenAPI3 that is missing in OpenAPI2.

Overall, a more modular approach could make maintenance easier and support both OpenAPI2 and OpenAPI3 more effectively.

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.

2 participants