OPENAPI-04: Add testing capabilities to verify and validate the generated OpenAPI specification#2
OPENAPI-04: Add testing capabilities to verify and validate the generated OpenAPI specification#2capernix wants to merge 8 commits intoopenmrs:mainfrom
Conversation
chibongho
left a comment
chibongho
left a comment
There was a problem hiding this comment.
I definitely haven't looked through everything yet, as this is a huge PR. Here are my comment.
| # OpenMRS REST OpenAPI Generator | ||
|
|
||
|  | ||
|  |
There was a problem hiding this comment.
This will, hopefully, be more than just a GSoC project. Can you remove this image, or replace with something more appropriate?
|  | ||
|
|
||
| A sophisticated Maven plugin that programmatically analyzes OpenMRS REST resources at build time to extract detailed representation metadata. This plugin uses a forked process approach to overcome Maven plugin classloader limitations and provides comprehensive analysis of REST resource handlers with their representation structures. | ||
| A Maven plugin that automatically generates **OpenAPI 3.0 specifications** from OpenMRS REST resources during the build process. This plugin uses runtime introspection to analyze REST resource handlers and produces comprehensive API documentation that's ready for tools like Swagger UI, Postman, or API clients. |
There was a problem hiding this comment.
| A Maven plugin that automatically generates **OpenAPI 3.0 specifications** from OpenMRS REST resources during the build process. This plugin uses runtime introspection to analyze REST resource handlers and produces comprehensive API documentation that's ready for tools like Swagger UI, Postman, or API clients. | |
| A Maven plugin that, when applied to OpenMRS modules, automatically generates **OpenAPI 3.0 specifications** for the module's REST resources during the build process. This plugin uses runtime introspection to analyze REST resource handlers and produces comprehensive API documentation that's ready for tools like Swagger UI, Postman, or API clients. |
| ## 🚀 What This Plugin Does | ||
|
|
||
| This plugin automatically discovers and analyzes **all OpenMRS REST resource handlers** in your project, extracting: | ||
| The plugin **automatically discovers and analyzes** OpenMRS REST resources in your module, generating: |
There was a problem hiding this comment.
These are the eventual goals of the plugin, but I think it's important that we:
- mention that it's a work in progress and not completed yet
- document what works (GET routes, the usual representations) and doesn't work (POST / DELETE routes, named representations) as of now. Avoid words like "Complete" until we are actually there.
Maybe also mention that, for now, this plugin has been tested against the REST module and queues module.
|
|
||
| ## 📊 Sample Output | ||
|
|
||
| The plugin generates a complete OpenAPI 3.0 specification like this: |
| ### 1. Install the Plugin | ||
|
|
||
| ### Installation | ||
| First, build and install the plugin to your local Maven repository: |
There was a problem hiding this comment.
Is this still accurate? We published the plugin, so no need to install locally right?
| private static JsonSchemaFactory schemaFactory; | ||
|
|
||
| // Known working endpoints from ResilientContractTest - guaranteed to be accessible | ||
| private static final List<String> GUARANTEED_WORKING_ENDPOINTS = Arrays.asList( |
There was a problem hiding this comment.
ambiguous what "working" means, maybe rename to KNOWN_ACCESSIBLE_ENDPOINTS?
| return resourceName.substring(0, 1).toUpperCase() + resourceName.substring(1).toLowerCase(); | ||
| } | ||
|
|
||
| private List<String> getAllEndpointsFromOpenApiSpec() { |
There was a problem hiding this comment.
Shouldn't this only return GET endpoints? If so, give it a better name.
Also, why do we need paths for both /ws/rest/v1/<resource> and /ws/rest/v1/<resource>/uuid? Isn't one of them sufficient for testing the GET representations?
| private String extractEndpointFromPath(String path) { | ||
| // Convert "/ws/rest/v1/concept/{uuid}" to "/concept" | ||
| if (path.startsWith("/ws/rest/v1/")) { | ||
| path = path.substring("/ws/rest/v1".length()); |
There was a problem hiding this comment.
Re-assigning to the input variable like this is confusing.
| } | ||
|
|
||
| // Remove {uuid} part | ||
| if (path.contains("/{uuid}")) { |
There was a problem hiding this comment.
Shouldn't this be else if?
Or better yet, can we just do early return?
return path.substring(0, path.indexOf("/{uuid}"));
| private List<String> getAvailableRepresentationsForEndpoint(String endpoint) { | ||
| Set<String> representations = new HashSet<>(); | ||
|
|
||
| // Skip problematic representations for known endpoints |
There was a problem hiding this comment.
Elaborate on the problems. Would be nice if the comment also links to relevant JIRA tickets
| /** | ||
| * Maps representation types to the corresponding method names that take delegate parameters | ||
| */ | ||
| private String getRepresentationMethodName(Representation representation) { |
There was a problem hiding this comment.
This is bad. We must not be depend on / assume the function name of the RepHandler. Loop through the methods of the class to find the @RepHandler function instead.
| } | ||
|
|
||
| // Phase 1 Enhancement: Made package-private for testing | ||
| Schema<?> mapToSwaggerSchema(String javaType, Components components, String representationHint, boolean isArrayItem) { |
There was a problem hiding this comment.
I found that the swagger library has a ModelConverters class for this kind of thing. We shouldn't need to map int / string / boolean / date types ourselves. See if we can rely on that API to handle more complex types as well.
|
|
||
| String resourceName = annotation.name(); | ||
| String resourcePath = "/ws/rest/" + resourceName + "/{uuid}"; | ||
| String resourcePath = "/ws/rest/" + RestConstants.VERSION_1 + "/" + resourceName + "/{uuid}"; |
There was a problem hiding this comment.
This incorrectly creates path with "v1" appearing twice, as in /ws/rest/v1/v1/patient
2 participants
Description of what I changed
This PR focused on wrapping up with test classes to validate and verify the generated OpenAPI spec to further prove it's working. We run a test on the REST module where we access all the REST endpoints, and use various third party libraries like JSONSchema and REST assured to validate each property against all representation types (Like ref, default, full). It also had a lot of improvements in the schema introspector implementation for various bugs.
Checklist: I completed these to help reviewers :)
[
x] My IDE is configured to follow the code style of this project.No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amend[] I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amend[
x] I ranmvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
[
x] All new and existing tests passed.No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
[
x] My pull request is based on the latest changes of the master branch.No? Unsure? -> execute command
git pull --rebase upstream master