RESTWS-963: Add a swagger autogeneration util to scan through resource handlers and generate the swagger spec #638

mherman22 · 2024-12-03T11:09:06Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-963

Checklist: I completed these to help reviewers :)

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
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
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.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

dkayiwa · 2024-12-03T11:25:53Z

It is a recommended practice to include the ticket id in the commit message.

dkayiwa · 2024-12-06T12:53:17Z

I was not expecting the new approach to change any existing methods. I expected to only inspect the resources to do the needful without touching them in any way.

mherman22 · 2024-12-06T14:12:27Z

I was not expecting the new approach to change any existing methods. I expected to only inspect the resources to do the needful without touching them in any way.

it doesn't change any existing method, to get this working i have to make changes within SwaggerSpecificationCreator.java. Except for the @Deprecated method, i am gonna add that back.

mherman22 · 2024-12-06T14:27:12Z

.../test/java/org/openmrs/module/unrelatedtest/rest/resource/UnrelatedGenericChildResource.java

+	public static boolean getRepresentationDescription = false;
+
 	@Override
-	public Model getGETModel(Representation rep) {


this is mainly to fix the test that depends on this class.

chibongho · 2025-03-10T14:15:56Z

@dkayiwa @ibacher Herman and I want to bring attention to this PR. It affects, among other things, the scope of the GSoC Project.

I haven't given this a thorough review, but this approach to stop relying on getGetModel(), getCREATEModel() and getUPDATEModel() makes sense . I don't think this PR introduces any backwards incompatibility. Given how incomplete our current swagger docs are, I think it's fairly low risk to merge this in, and make continual improvements on it.

dkayiwa · 2025-03-10T20:28:11Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/RestService.java

+	 * @param resourceClass the resource class e.g PatientIdentifier.class
+	 * @return the resource handler for the provided class e.g PatientIdentifierResource1_8
+	 */
+	Resource getResourceHandlerForSupportedClass(Class<?> resourceClass);


Where are you using this method?

To be removed. Oversight

So this method handles cases where the field is a List or Set. It extracts the element type and, if it's an OpenMRS resource, references its Swagger definition. If no resource definition is found, it defaults to a StringProperty. Otherwise, it returns a generic ArrayProperty.

if (Set.class.equals(type) || List.class.equals(type)) { Class<?> elementType = getGenericTypeFromField(field); if (isOpenMRSResource(elementType)) { String resourceName = getSubResourceNameBySupportedClass(elementType); if (resourceName == null) { return new StringProperty(); } return new ArrayProperty(new RefProperty("#/definitions/" + StringUtils.capitalize(resourceName) + operationType)); } return new ArrayProperty(); }

cc: @dkayiwa

Can you point me to where it is currently used?

https://github.com/mherman22/openmrs-module-webservices.rest/blob/bcaf91a7c68266080580c33181899bff9a95b6bd/omod-common/src/main/java/org/openmrs/module/webservices/docs/swagger/SwaggerGenerationUtil.java#L286-L309

Do we need to put it here if it is only used in a utility class?

not necessarily, i can just put it within the SwaggerGenerationUtil.java

dkayiwa · 2025-03-10T20:46:10Z

omod-2.0/src/test/java/org/openmrs/module/webservices/rest/resource/SubDetailsResource.java

-
+
+	@Override
+	public DelegatingResourceDescription getCreatableProperties() throws ResourceDoesNotSupportOperationException {


Why did we add this one?

This class has a getCREATEModel method that needs to be removed. I wanted to replace that with getcreatableproperties since the new mechanism depends on that

I do not seem to see where you removed it in this pull request.

True dat. Will fix that

Did you address the above?

I tried addressing it in this commit but it fails some tests in swaggerspecificationcreatortest so I decided to put it back in this most recent commit.

Its really because we need that method to add SubDetailsResource to the swagger documentation. When I remove it, the tests fail because that resource is not added

I still somehow feel we should not add this new method just for the sake of the swagger docs.

Shouldn't we just replace the test with one adapted to this new model? I.e., if getCREATEModel() is no longer what Swagger is using to create docs, then the test is no longer valid?

@dkayiwa @ibacher, Are you suggesting i remove even the SubDetailsResource? because that's the only way the existing tests especially this one will pass?.

And to be frank, SubDetailsResource is just a test class that shouldn't appear in our swagger spec but apparently does so i am okay with removing it.

Hmmm... it shouldn't appear in the production spec, but I thought it was there so the tests have something to work with. I don't think we should remove it, but I do think we should re-work the SwaggerSpecificationCreatorTest class so that it tests whatever mechanism we're using to generate the Swagger Specification now.

We don't need existing tests to pass if we're changing the mechanism, but we need an equivalent set of tests to pass. Does that make sense?

Also, when actually generating the spec, we should ensure that none of the test modules are on the classpath.

dkayiwa · 2025-03-16T21:48:23Z

.../test/java/org/openmrs/module/unrelatedtest/rest/resource/UnrelatedGenericChildResource.java

 	@Override
-	public Model getGETModel(Representation rep) {
+	public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
 		getGETCalled = true;


Did you intentionally leave in the getGETCalled?

yes, these may stay mainly because of this test -> https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-2.0/src/test/java/org/openmrs/module/webservices/rest/doc/SwaggerSpecificationCreatorTest.java#L290-L315

dkayiwa · 2025-03-16T21:48:40Z

.../test/java/org/openmrs/module/unrelatedtest/rest/resource/UnrelatedGenericChildResource.java

 	@Override
-	public Model getCREATEModel(Representation rep) {
+	public DelegatingResourceDescription getCreatableProperties() {
 		getCREATECalled = true;


Did you intentionally leave in the getCREATECalled?

yes, these may stay mainly because of this test -> https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-2.0/src/test/java/org/openmrs/module/webservices/rest/doc/SwaggerSpecificationCreatorTest.java#L290-L315

dkayiwa · 2025-03-16T21:48:54Z

.../test/java/org/openmrs/module/unrelatedtest/rest/resource/UnrelatedGenericChildResource.java

 	@Override
-	public Model getUPDATEModel(Representation rep) {
+	public DelegatingResourceDescription getUpdatableProperties() throws ResourceDoesNotSupportOperationException {
 		getUPDATECalled = true;


Did you intentionally leave in the getUPDATECalled?

To be fixed in next commit

dkayiwa · 2025-03-16T21:51:26Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImpl.java

 		}
 	}
-
-	private void initializeResources() {


Do you mind explaining why you moved this from a private method to the base interface?

Oversight. To be fixed in next commit

scratch that, it was actually intentional to move it to the base interface because i need to use it in this method as can be seen at https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-common/src/main/java/org/openmrs/module/webservices/docs/swagger/SwaggerGenerationUtil.java#L408

cc: @dkayiwa

Is there any reason why you did not just use RestService.initialize()?

dkayiwa · 2025-03-17T21:32:33Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImpl.java


 	volatile Map<String, ResourceDefinition> resourceDefinitionsByNames;

-	volatile Map<Class<?>, Resource> resourcesBySupportedClasses;


Did you intentionally change the access modifier?

Yes, being used here https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-common/src/main/java/org/openmrs/module/webservices/docs/swagger/SwaggerGenerationUtil.java#L407

It is not a good practice for outsiders to directly access internal implementation details of a class?

True dat. Let me get a finer way of doing it

dkayiwa · 2025-03-19T19:15:34Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImpl.java

-	
-	volatile Map<Class<?>, Resource> resourcesBySupportedClasses;
+
+	static volatile Map<Class<?>, Resource> resourcesBySupportedClasses;


Originally, this was not static. Is this just a typo?

Did you see the above comment?

dkayiwa · 2025-03-19T19:17:21Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/RestService.java

+
+	/**
+	 * Retrieves the resource handler for the given supported class.
+	 *


Don't we already have a way of getting this without modifying this interface by adding a new method simply for swagger docs?

i have been trying out this method https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImpl.java#L440 but it is not giving me what i want.

i dont want to touch the existing tests in https://github.com/mherman22/openmrs-module-webservices.rest/blob/08b817fd98e0c15f81f021d4e856b0c2b598c125/omod-2.0/src/test/java/org/openmrs/module/webservices/rest/doc/SwaggerSpecificationCreatorTest.java#L65

And yet when i use the above method i get the failure below: https://pastebin.com/WknBFXSD which is that it fails with Caused by: org.openmrs.api.APIException: Unknown resource: class org.openmrs.AllergyReaction. AllergyReaction is a dummy class which was added just for the sake of making SwaggerSpecificationCreatorTest pass.

What parameter did you pass into getResourceBySupportedClass?

here is the method. i gave it a Class<?> resourceClass.

/** * <strong>Should</strong> return search resources for given resource class * @param resourceClass the resource class e.g. PatientIdentifier */ public static org.openmrs.module.webservices.rest.web.resource.api.Resource getResourceHandlerForSupportedClass(Class<?> resourceClass) { if (Context.getService(RestService.class).getResourceBySupportedClass(resourceClass) == null) { Context.getService(RestService.class).initialize(); } return Context.getService(RestService.class).getResourceBySupportedClass(resourceClass); }

What was the exact value contained in resourceClass?

@dkayiwa i have added a fix by simply using getResourceHandlers() and then use each of the returned resourcehandlers to get its annotation and ensure that the annotation supportedClass is equal to the class i am providing at public static String getSubResourceNameBySupportedClass(Class<?> supportedClass).

…rs and generate the swagger spec

… making it public

…ution

mherman22 force-pushed the RESTWS-963-Impl branch 2 times, most recently from 67dbf47 to a3de500 Compare December 3, 2024 20:00

mherman22 requested a review from dkayiwa December 5, 2024 17:54

mherman22 commented Dec 6, 2024

View reviewed changes

mherman22 force-pushed the RESTWS-963-Impl branch 2 times, most recently from e234eac to 9a6ee8e Compare January 11, 2025 19:25

mherman22 force-pushed the RESTWS-963-Impl branch 2 times, most recently from b00e48b to fb399b3 Compare January 31, 2025 05:45

dkayiwa reviewed Mar 10, 2025

View reviewed changes

mherman22 requested a review from dkayiwa March 16, 2025 11:25

dkayiwa reviewed Mar 16, 2025

View reviewed changes

dkayiwa reviewed Mar 17, 2025

View reviewed changes

dkayiwa reviewed Mar 19, 2025

View reviewed changes

mherman22 requested a review from dkayiwa March 22, 2025 15:28

mherman22 force-pushed the RESTWS-963-Impl branch 3 times, most recently from f222c40 to d60be17 Compare March 26, 2025 15:20

mherman22 force-pushed the RESTWS-963-Impl branch 3 times, most recently from 7f68a6c to fae9fdc Compare May 25, 2025 10:48

RESTWS-963: Add a swagger autogeneration util to scan resource handle…

65d3181

…rs and generate the swagger spec

mherman22 added 9 commits July 2, 2025 12:00

fix some of the reviews

7fc49c5

fix failing tests

cdabbe4

remove initializeResources from base interface

657a0df

Encapsulate resource handler map by adding a getter method instead of…

b17d463

… making it public

remove static

2765fab

use getResourceHandlers() instead

2306c24

update some tests to match new approach

d3c3699

more tests

b067d94

ignore addPaths_shouldWorkForCoreModels as i figure out a lasting sol…

5b01a90

…ution

mherman22 force-pushed the RESTWS-963-Impl branch from fae9fdc to 5b01a90 Compare July 2, 2025 09:04



		@Override
		public DelegatingResourceDescription getCreatableProperties() throws ResourceDoesNotSupportOperationException {


		volatile Map<String, ResourceDefinition> resourceDefinitionsByNames;

		volatile Map<Class<?>, Resource> resourcesBySupportedClasses;


		volatile Map<Class<?>, Resource> resourcesBySupportedClasses;

		static volatile Map<Class<?>, Resource> resourcesBySupportedClasses;

RESTWS-963: Add a swagger autogeneration util to scan through resource handlers and generate the swagger spec #638

Are you sure you want to change the base?

RESTWS-963: Add a swagger autogeneration util to scan through resource handlers and generate the swagger spec #638

Uh oh!

Conversation

mherman22 commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

dkayiwa commented Dec 3, 2024

Uh oh!

dkayiwa commented Dec 6, 2024

Uh oh!

mherman22 commented Dec 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chibongho commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mherman22 commented Dec 3, 2024 •

edited

Loading

dkayiwa Mar 24, 2025 •

edited

Loading

dkayiwa Mar 17, 2025 •

edited

Loading