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

Support additional API Versioning strategies #3008

Closed
wants to merge 42 commits into from

Conversation

justin-tay
Copy link
Collaborator

Description

Adds support for the following API Versioning strategies

  • Path
  • Header
  • Parameters
  • Media Type Profile

This can be customized by implementing and registering a com.yahoo.elide.core.request.route.RouteResolver.

The default in Elide Spring Boot is now using the Path strategy instead of using the ApiVersion header. The Path strategy is the only one that is supported when integrating with Springdoc as the other strategies are difficult to document with OpenAPI.

This can be configured back to the previous defaults using application.yaml.

The default in Elide Standalone now accepts all the strategies and can be configured to previous defaults with the getRouteResolver method.

Modifications were also made to ensure the entities for different API versions can be in the same OpenAPI document. This also adds GroupedOpenApi support for Springdoc.

The following API changes were also made

  • The com.yahoo.elide.core.security.RequestScope interface has changed to be able to return a Route. This replaces the getApiVersion(), getRequestHeaderByName(), getBaseUrlEndPoint() and getQueryParams() methods.
  • The JSON-API relevant fields in com.yahoo.elide.core.RequestScope have been moved to com.yahoo.elide.jsonapi.JsonApiRequestScope.
  • The JSON-API processing logic in com.yahoo.elide.Elide have been moved to com.yahoo.elide.jsonapi.JsonApi.
  • The ElideSettingsBuilder has been changed to be more consistent with the other Lombok builders and to allow easier customization over the defaults instead of replacing it.

Motivation and Context

Previously only the Header API Versioning strategy was supported it wasn't easy to customize to add other strategies. The other changes are also to make Elide easier to customize.

How Has This Been Tested?

Added the relevant unit and integration tests. Have also tested this against the example apps.

License

I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@aklish
Copy link
Member

aklish commented Jun 11, 2023

Love the PR. Small comment that will help me move these along faster is to break this up into different PRs. Maybe name changes can be done in one PR - separate from the meat. Name changes are a lot of lines, but quick and easy to review. I know this isn't always possible - but the sweet spot would be 500 lines or less in one go.

Also helpful is some signal when the PR is ready for review.

@justin-tay
Copy link
Collaborator Author

Love the PR. Small comment that will help me move these along faster is to break this up into different PRs. Maybe name changes can be done in one PR - separate from the meat. Name changes are a lot of lines, but quick and easy to review. I know this isn't always possible - but the sweet spot would be 500 lines or less in one go.

Also helpful is some signal when the PR is ready for review.

Noted your comments. I will try to break this PR up.

Thanks for all the hard work doing the reviews, merges and releases. It's much appreciated.

It's a little tricky to break this PR up. I think what I will do is convert this to a draft and create the smaller change PRs and when those get merged I will rebase this one until it is small enough.

@justin-tay justin-tay marked this pull request as draft June 12, 2023 03:19
@justin-tay justin-tay force-pushed the apiversion branch 3 times, most recently from b6f6dab to 27803d8 Compare June 20, 2023 03:18
@justin-tay justin-tay marked this pull request as ready for review June 23, 2023 08:54
@justin-tay justin-tay requested a review from aklish June 23, 2023 08:55
@justin-tay justin-tay force-pushed the apiversion branch 4 times, most recently from 970e38e to 6e1e114 Compare July 24, 2023 01:41
int pathEnd = -1;

int find = path.indexOf('/', 0);
if (find != -1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simpler with a regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I didn't profile it, since this essentially called for every request, I thought that doing string operations might be faster than a regex.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. The logic looks correct to me.

@aklish
Copy link
Member

aklish commented Aug 30, 2023

Suggestion for how to start breaking this up. Let's separate the Resolvers & validators and their tests as a starting PR. I realize nothing will call these resolvers, but I think if we can break this into chunks, we can get this reviewed and merged.

@aklish
Copy link
Member

aklish commented Aug 30, 2023

After the resolvers, maybe we can separate just the ElideSettingsBuilder changes as its own PR.

@justin-tay justin-tay marked this pull request as draft August 30, 2023 05:45
@justin-tay
Copy link
Collaborator Author

These changes have been merged in separate PRs.

@justin-tay justin-tay closed this Oct 23, 2023
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.

None yet

2 participants