Skip to content

Conversation

capernix
Copy link
Contributor

@capernix capernix commented May 20, 2025

Description of what I changed

Introduced a new OpenAPI Generator Maven Plugin that integrates JavaParser to extract Javadoc from OpenMRS REST resource classes. This initial implementation serves as a proof-of-concept for enhancing OpenAPI documentation with descriptive information from source code.

Key Implementation Details:

  1. Created a custom Maven plugin (openapi-generator-maven-plugin) with JavaParser integration
  2. Implemented source file location logic to find Java classes across module source roots
  3. Added functionality to parse Java source files and extract class-level Javadoc comments
  4. Successfully tested extraction of Javadoc from PatientResource1_8 class
  5. Fixed a Maven build dependency issue by moving the unpack-dependencies goal from generate-resources to prepare-package phase

This implementation provides the foundation for programmatically enhancing API documentation by leveraging the existing Javadoc in the codebase.

PS: I also pushed the code for the schema introspector alongside so the work can be combined in future commits.

image (1)

Issue I worked on

https://openmrs.atlassian.net/browse/RESTWS-980

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 ran mvn clean package right before creating this pull request and
    added 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

@capernix capernix marked this pull request as draft May 20, 2025 17:16

<groupId>org.openmrs.plugin</groupId>
<artifactId>openapi-generator-maven-plugin</artifactId>
<version>1.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

@capernix i know this is still in draft but have a look at how versioning is done in OpenMRS, -> https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477712/Versioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look thanks

@capernix
Copy link
Contributor Author

About the Commit: Enhanced the Plugin to invoke the required function and parse representations

Created a Maven Plugin that currently sets up the URLClassLoader, tests if Rest Constants are accessible, loads the specific resources and invokes getRepresentationDescription() to gather all the properties of the resource.

@Parameter(defaultValue = "${project}", required = true, readonly = true)
private MavenProject project; @Override
public void execute() throws MojoExecutionException, MojoFailureException {
getLog().info("=== OPENAPI GENERATION STARTED ===");
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer you declaring something like private static final Logger logger = LoggerFactory.getLogger(OpenAPIMojo.class); and replace this line with something like logger.info(.....);

Copy link
Member

Choose a reason for hiding this comment

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

Do the same for all your logging needs

omod-1.8/pom.xml Outdated
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<configuration>
<artifactId>license-maven-plugin</artifactId> <configuration>
Copy link
Member

Choose a reason for hiding this comment

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

Is this change part of your plugin implementation?

omod-1.8/pom.xml Outdated
<plugin>
<groupId>org.openmrs.plugin</groupId>
<artifactId>openapi-generator-maven-plugin</artifactId>
<version>1.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

This versioning doesnt match openmrs' style of versioning, look at https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477712/Versioning


<dependencies>
<!-- Maven Plugin API -->
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see classLoader.loadClass used quite a bit to load classes from the rest module. Does it make sense to just make it a dependency so we can use those classes directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try this out and get back

}

} catch (Exception e) {
getLog().error("Method invocation failed: " + e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid this catch-and-log pattern. It hides the stack trace. If the error is unrecoverable, just let it be thrown without catching.

Also, try to avoid catching Exception, as it is too general. You might mask genuine coding errors like IndexOutOfBoundsException.

Class<?> resourceClazz = loadClass(TARGET_CLASS_NAME);

// Test PatientResource instance creation
testPatientResourceInstantiation(resourceClazz);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ready to make this work for all Resources classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to my observations and testing, almost all of the resource files are working perfectly.
But an exception I found was ConceptResource1_8 that has it's full representation in a different method than getRepresentationDescription(), and it has additional representations such as fullchildren which I am working on to be taken into consideration.

@capernix capernix force-pushed the RESTWS-980 branch 2 times, most recently from 582488f to afd8d00 Compare July 1, 2025 17:16
Copy link
Member

@mherman22 mherman22 left a comment

Choose a reason for hiding this comment

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

@capernix , have you seen the build failure. Can you investigate it?

import java.util.Map;

@Mojo(name = "openapi", defaultPhase = LifecyclePhase.PROCESS_CLASSES, requiresDependencyResolution = ResolutionScope.RUNTIME)
public class OpenAPIMojo extends AbstractMojo {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class OpenAPIMojo extends AbstractMojo {
public class OpenmrsOpenApiSpecMojo extends AbstractMojo {

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 that name makes more sense

return new URLClassLoader(urls.toArray(new URL[0]), Thread.currentThread().getContextClassLoader());

} catch (java.net.MalformedURLException e) {
throw new RuntimeException("Invalid URL in classpath elements: " + e.getMessage(), e);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we prefer throwing MojoExecutionException or MojoFailureException for plugin errors, as Maven will display these more clearly?

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.

3 participants