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

openapi-v3 #9550

Merged
merged 17 commits into from
Apr 18, 2024
Merged

openapi-v3 #9550

merged 17 commits into from
Apr 18, 2024

Conversation

david-leifker
Copy link
Collaborator

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@@ -92,6 +92,10 @@ public MergedEntityRegistry apply(EntityRegistry patchEntityRegistry)
}
}

if (!patchEntityRegistry.getAspectSpecs().isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be duplicated from above, if this needs to run after the merge entity specs block then first one should be removed.

assertEquals(openAPI.getComponents().getSchemas().size(), 914);
assertEquals(openAPI.getComponents().getParameters().size(), 56);
assertEquals(openAPI.getPaths().size(), 102);
assertTrue(openAPI.getComponents().getSchemas().size() >= 882);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like an odd spot to have varying numbers, I think we'd want to know if new schemas are showing up? Maybe it's not important though if this will change often.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They would change on every data model update including changes to the entity-registry. The numbers are going to increase fairly constantly unless we start deprecating things. A simple non-zero check would however not be as useful since it might actually be broken for large chunks of the data models so this is trying to detect a regression rather then ensuring perfect accuracy.

* json schema validator update
* tests fixes, timeline, counts
review comment addressed
@@ -18,7 +18,8 @@
"io.datahubproject.openapi.v1",
"io.datahubproject.openapi.v2",
"io.datahubproject.openapi.v3",
"com.linkedin.gms.factory.timeline"
"com.linkedin.gms.factory.timeline",
"org.springdoc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why was this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The org.springdoc package includes the @Controller classes that create the swagger UI endpoints. Without Spring able to detect those controllers they never get wired up.

NONDEFAULT_OPENAPI_PACKAGES.addAll(V1_PACKAGES);
NONDEFAULT_OPENAPI_PACKAGES.addAll(V2_PACKAGES);
NONDEFAULT_OPENAPI_PACKAGES.addAll(V3_PACKAGES);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the new default openapi page will now have all of the above packages right? AKA we don't want to hide them anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The difference is there is actually no longer a catch-all group. If new endpoints are added they must either fall into an existing group or a new group can be added. This is more explicit and avoids wiring up endpoints inadvertently with some misplaced annotations.

@david-leifker
Copy link
Collaborator Author

The python tests are failing for some reason, will be resolved with #10318

@david-leifker david-leifker merged commit adffce2 into master Apr 18, 2024
64 checks passed
@david-leifker david-leifker deleted the feature/data-platform-1 branch April 18, 2024 20:03
sleeperdeep pushed a commit to sleeperdeep/datahub that referenced this pull request Jun 25, 2024
Co-authored-by: Ajoy Majumdar <[email protected]>

Adds support for custom aspects in the openapi api
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