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

docs: rewrite API first guide with detailed implementation and samples #1335

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

timothystone-knsl
Copy link
Contributor

@timothystone-knsl timothystone-knsl commented Feb 4, 2024

Re-ordered concepts to introduce concepts provided by JHipster with details on the out of the box configuration of the openapi-generator and the use of the Delegate Pattern. Explicitly callout the use of the OAI's Pet Store example with links to samples.

Added notes on JHipster's technical structure and the use of ArchUnit to prepare the reader for the implementation of the API operations.

Rewrite and editdocs for a detailed introduction of API first development

NOTE: removed references to the Maven IDEA and Eclipse plugins. Both have been retired and should be removed from JHipster.

Added detailed examples of the implementation of NativeWebRequest and responses

Lots of examples and details added to both the basic and detailed implementation with mock request responses prior to implementation with the NativeWebRequest.

Add sample api.yml used in guide and build out initial content

The content is largely complete and ready for review. Add links and the sample api.yml document used throughout the guide.

Closes jhipster/generator-jhipster#25030

Copy link

netlify bot commented Feb 4, 2024

Deploy Preview for jhipster-site ready!

Name Link
🔨 Latest commit 163f42b
🔍 Latest deploy log https://app.netlify.com/sites/jhipster-site/deploys/66d0a958af3c750008e9df1e
😎 Deploy Preview https://deploy-preview-1335--jhipster-site.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@timothystone-knsl timothystone-knsl marked this pull request as ready for review February 4, 2024 17:14
timothystone-knsl added a commit to timothystone-knsl/generator-jhipster that referenced this pull request Feb 4, 2024
Both plugins have been retired by Apache Maven. See: https://maven.apache.org/plugins/#retired for
more details. Both plugins have not see updates in almost a decade (IDEA was last updated in 2013).

The mention and use of the plugins has been removed from the Doing API First Development guide in
jhipster/jhipster.github.io#1335.

Closes jhipster#25081
@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Feb 6, 2024

@mraible see above deployment and attempt navigation to the proposed changes in Options > Doing API First Development. The redirect is broken an in an infinite loop. See also the issue and this comment for more context, jhipster/generator-jhipster#25030 (comment).

I'm alerting you because I just found this in the Netlify deployment checks... you or somebody on the team may wish to look into the redirect looping.

image

@mraible
Copy link
Collaborator

mraible commented Feb 6, 2024 via email

@timothystone-knsl
Copy link
Contributor Author

Does it work when you run it locally? I've seen this happen with other pages, but I'm not sure how to fix it.

I haven't went that far yet... but I'll try today, time permitting. Thanks for looking! Sorry to pick on you... but I saw your name "Matt Raible's Team" and decided it was worth a tag.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Feb 7, 2024

Moving the conversation from the jhipster/generator-jhipster#25030.

Basically a full rewrite of the API First document with sample files, consistent examples, and explanations for new comers. Includes configuration of the plugins and sample code showing full context. Removed references to retired tooling, see jhipster/generator-jhipster#25081 (merged). Added authentication tips (and a couple of Pro Tips).

Since the Netlify deployment doesn't work, the edits can be reviewed in context here.

@timothystone-knsl
Copy link
Contributor Author

@mraible @atomfrede who watches and reviews these JHipster.tech PRs? Anyone I should tag?

@mraible
Copy link
Collaborator

mraible commented Feb 13, 2024

Hi @timothystone-knsl I'm currently on vacation and don't have time to review. It's possible I will in the coming weeks.

@atomfrede
Copy link
Member

Sorry @timothystone-knsl I wanted to have a look. Will try it this week.

@timothystone-knsl
Copy link
Contributor Author

I too just returned from vacation. Just poking this PR.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented May 3, 2024

@mraible @atomfrede I'm still interested in making this guide more approachable and removing what I see as assumption gaps in the existing guide.

One thing I need to resolve and it's not obvious to me is that the OpenAPI Generator as configured to useDelegates=true requires an implementation.

In the guide (blob link), it is explicitly called out:

Providing the NativeWebRequest bean to the Delegate interface can return example response bodies for the methods that have not been overridden. The endpoints still respond with a 501 Not Implemented HTTP status code, but the example response may be useful for mocking endpoints before the actual implementation.

This may be true, but the generated code does not resolve this "provided NativeWebRequest bean" in the IDE. One of the things I'm trying to do with the rewrite is to close some assumption gaps, where the assumptions are made by experienced users of the frameworks and tooling as configured by default in JHipster, i.e., delegatePattern=true (where this default is even debated in some forums as necessary or "unnecessary complexity").

How can I help close this gap and provide the necessary tips or instructions for new users of JHipster in "providing the bean" properly?

Copy link
Member

@atomfrede atomfrede left a comment

Choose a reason for hiding this comment

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

Awesome work @timothystone-knsl. It really reads great.

pages/doing_api_first_development.md Outdated Show resolved Hide resolved
pages/doing_api_first_development.md Outdated Show resolved Hide resolved
@atomfrede
Copy link
Member

@mraible @atomfrede I'm still interested in making this guide more approachable and removing what I see as assumption gaps in the existing guide.

One thing I need to resolve and it's not obvious to me is that the OpenAPI Generator as configured to useDelegates=true requires an implementation.

In the guide (blob link), it is explictly called out:

Providing the NativeWebRequest bean to the Delegate interface can return example response bodies for the methods that have not been overridden. The endpoints still respond with a 501 Not Implemented HTTP status code, but the example response may be useful for mocking endpoints before the actual implementation.

This may be true, but the generated code does not resolve this "provided NativeWebRequest bean" in the IDE. One of the things I'm trying to do with the rewrite is to close some assumption gaps, where the assumptions are made by experienced users of the frameworks and tooling as configured by default in JHipster, i.e., useDelegate=true (where this default is even debated in some forums as necessary or "unnecessary complexity").

How can I help close this gap and provide the necessary tips or instructions for new users of JHipster in "providing the bean" properly?

What is the issue with the nativeWebRequets and the IDE? When it comes to delegate use, this could be changed if we agree its not needed. At least in my projects (not jhipster ones) we have always used delegates as far as I remember.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented May 3, 2024

What is the issue with the nativeWebRequests and the IDE? When it comes to delegate use, this could be changed if we agree its not needed. At least in my projects (not jhipster ones) we have always used delegates as far as I remember.

I don't have a real issue with the use of the delegate pattern. Where the discussion occurs it is around preferences and is mostly a question for me of: is JHipster providing the pattern as an example of best practice or if interfaceOnly would be "more approachable." The delegatePattern=true is not the default, but delegatePattern=false so JHipster is making a deliberate decision here.

In IntelliJ, one will often see this:

image

It doesn't seem to impact anything... at least casually, but I've been asked about it and don't have an answer. My Spring experience has its own gaps and I have not been able to resolve this myself. It feels as if the OpenAPI generator is not doing something and there is a gap in the documentation that can address it.

@atomfrede
Copy link
Member

Good point. I really can not recall any explicit decision for delagate pattern. If we do not have one we should adhere to our policy to use technologies with their default config (when possible)

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented May 4, 2024

Good point. I really can not recall any explicit decision for delagate pattern. If we do not have one we should adhere to our policy to use technologies with their default config (when possible)

On that, the following would need to be changed or removed in the generator. See also the documentation for the Spring Generator defaults.

If there as simple Spring configuration fix that would address the "missing bean" in the IDE, whether a PR in the OpenAPI plugin or in the src/main/java/**/config package I'm happy to take that on as part of both the documentation PR and in the generator.

@mraible
Copy link
Collaborator

mraible commented May 10, 2024

@timothystone-knsl I don't know much about this as I haven't used this feature. A PR would be most welcome if it makes things better.

Re-ordered concepts to introduce concepts provided by JHipster with details on the out of the box
configuration of the openapi-generator and the use of the Delegate Pattern. Explicitly callout the
use of the OAI's Pet Store example with links to samples.

Added notes on JHipster's technical structure and the use of ArchUnit to prepare the reader for the
implementation of the API operations.

Rewrite and editdocs for a detailed introduction of API first development

NOTE: removed references to the Maven IDEA and Eclipse plugins. Both have been retired and should be
removed from JHipster.

Added detailed examples of the implementation of NativeWebRequest and responses

Lots of examples and details added to both the basic and detailed implementation with mock request
responses prior to implementation with the NativeWebRequest.

Add sample api.yml used in guide and build out initial content

The content is largely complete and ready for review. Add links and the sample api.yml document used
throughout the guide.
JHipster defaults the `/api/**` behind authentication via the SecurityConfigurationFilter chain,
e.g., `.authenticated()`. It is possible to craft a JWT Bearer token to test the API using the JWT
Secret generated by JHipster during creation.
Test the desired URL to help Jekyll with redirects
Update docs to reflect OpenAPI implementation recommendation.
The openapi-client subgenerator is no longer maintained after v8.

```shell
 % jhipster openapi-client
WARNING! Since JHipster v8, the jhipster command will not use the locally installed generator-jhipster.
    If you want to execute the locally installed generator-jhipster, run: npx jhipster
FATAL!

This sub-generator is no longer maintained and has been removed from JHipster starting from v8.
If you are interested in maintaining this sub-generator fill a feature request on the JHipster bug tracker.
```
@timothystone-knsl
Copy link
Contributor Author

Updated, removing the old note on using jhipster openapi-client subgenerator as that is no longer maintained since v8.

I might ask a separate question: why have the option in jhipster --help? Maybe allow the use for those looking to use it and report the error as today, but not list it in the usage output?

Generically state the configuration of the project for API First tooling and link to the OpenAPI
Generator page for more information.
@mraible mraible requested a review from atomfrede June 26, 2024 00:35
Copy link
Member

@atomfrede atomfrede left a comment

Choose a reason for hiding this comment

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

Just minor things I noticed. Besides that the overall structure and content is really nice. It will provide a great starting point for people that have never use openapi generator before.

pages/doing_api_first_development.md Outdated Show resolved Hide resolved
└── PetsApiDelegate.java
```

> ⚠️The OpenAPI Generator is responsible for the creation of DTOs ("models") and delegates. Implementations MUST not be placed in the `target/generated-sources`. These classes are subject to generation at anytime and SHOULD NOT be committed to source control.
Copy link
Member

Choose a reason for hiding this comment

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

Confusing, MUST not be place in target and should not be comitted to source control. Do you mean MUST be placed in target/generatedSources?

Copy link
Contributor

@timothystone timothystone Aug 31, 2024

Choose a reason for hiding this comment

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

"MUST NOT"... Implementations should be put into the src/main/java (project.build.sourceDirectory) and the appropriate path to meet the technical structure tests.

The target (project.build.directory) should be considered "ephemeral" and is deleted when running mvn clean. If the implementation code is put in the target directory, it has to be restored, i.e., git co target/... or something similar any time the clean lifecycle is run. And that means one has to commit this ephemeral code. 🫣 Running mvn clean might be performed fairly often as the generator is bound to the generate-sources phase of the default lifecycle and does not overwrite existing generated classes from the OpenAPI Specification ("swagger") document, api.yml.

Since implementation code would be part of the project, one would not want it deleted every time the plugin needs to update the generated source.

pages/doing_api_first_development.md Outdated Show resolved Hide resolved
@mraible
Copy link
Collaborator

mraible commented Aug 29, 2024

We updated the website with a new design. Can you please update this issue accordingly?

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Aug 29, 2024

We updated the website with a new design. Can you please update this issue accordingly?

Absolutely, working on it.

One outstanding question may need to be revisited soon: is <delegatePattern>true</delegatePattern> the correct setting for JHipster? It is not the default of the org.openapitools:openapi-generator-maven-plugin, see comment.

…i-first

Merge new website content and resolve conflicts.
@timothystone-knsl
Copy link
Contributor Author

@mraible done.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Aug 29, 2024

@mraible hold. reorganization of files broke a link in the guide to the sample api.yml. fixing. Fixed.

Do you want this branched squashed, or will you do it on merge?

Linking static file for reference.
Following Docusaurus documentation. Create a directory in /static/<reference-file>/path

Update path.
@mraible
Copy link
Collaborator

mraible commented Aug 29, 2024

Do you want this branched squashed, or will you do it on merge?

I usually squash and merge. But you're welcome to do it in advance if you like.

@timothystone-knsl
Copy link
Contributor Author

You have the conn sir!

@timothystone
Copy link
Contributor

We updated the website with a new design. Can you please update this issue accordingly?

Absolutely, working on it.

One outstanding question may need to be revisited soon: is <delegatePattern>true</delegatePattern> the correct setting for JHipster? It is not the default of the org.openapitools:openapi-generator-maven-plugin, see comment.

@atomfrede @DanielFran any strong opinion on the use of the delegate pattern over the default of the openapi-generator-maven-plugin? This guide could be easily reworked.

@atomfrede
Copy link
Member

No strong opinion. For me the delegate pattern is fine as I used it myself a lot with the generator.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Sep 4, 2024

No strong opinion. For me the delegate pattern is fine as I used it myself a lot with the generator.

Ditto. Mostly because the default is to use it. However, I have found that testing the implementation classes is not well documented. Do you have examples @atomfrede you can share or point to in OSS? There could be a lot of value in this document for a "Testing" section.;

Update I have some, but mine are minimal: to pass Sonar. And I can never seem get them to use the implementation properly. Inexperience? Documentation misunderstanding? Either way, it's a bit of a gap in the documentation. I wouldn't ask otherwise, but my Google-fu is not up to the task. :/

@timothystone
Copy link
Contributor

timothystone commented Sep 5, 2024

I've got some more work in this guide I think.

The original guide made no mention on how to use the DTOs generated by the OpenAPI specification and the repositories. Using JDL in conjunction with API-first development is largely incompatible.

OpenAPI generates the Delegate and Controllers along side the DTOs. But the Mappers and Services are not. I'm looking at the configuration of the openapi-generator-maven-plugin options, but not finding the support out of the box.

If I use JDL, for example:

application {
  config {
    baseName books
    applicationType microservice
    packageName com.anothercaffeinatedday.books
    authenticationType jwt
    prodDatabaseType postgresql
    devDatabaseType h2Disk
    serviceDiscoveryType no
    cacheProvider ehcache
  }

  entities Book
  use mapstruct for Book
}

entity Book {
  id Long
  title String required
  isbn String minlength(10) maxlength(10) pattern(/^\d{10}$/)
  totalPages Integer max(1500)
  rating Double min(1.0) max(5.0)
  publishedDate LocalDate
}

This generates the accompanying services, resources, mappers, repositories, &tc.

If I want to really defer to the generator, I think I need to add information on creating the services and mappers leaving the DTOs to the generator.

Have any of the reviewers dived into this with the guide as is?

@timothystone
Copy link
Contributor

timothystone commented Sep 7, 2024

@mraible @atomfrede I think I sorted it out. I'll move the effort to another PR and issue so as to close this thread and merge this to the docs.

API first development means something specific

Please let me know if anything here is incorrectly using JHipster JDL to achieve the goal of API First development.

IMHO, for true contract first development—as provided by the Maven plugin and the OpenAPI Specification—one will have to turn off and turn on some behaviors in the JDL to defer to the openapi-generator-maven-plugin. The plugin will perform some specific things in conflict with JHipster's default behaviors. At the very least, the plugin in will:

  1. Generate the "controllers and delegates" (JHI's generation of the "*Resource.java" should be turned off)
  2. Generate DTOs (JHI's generation of DTOs should be turned off)

However, the openapi-generator-maven-plugin is NOT going to generate the MapStruct mappers and service classes. This is not documented.

I achieved my understanding of API First development with the following JDL:

application {
  config {
    ...
    enableSwaggerCodegen true
  }
...
  entities *
  use mapstruct for *
}

This will leave the Delegate Implementation largely on the developer. Because skipServer * is not going to generate the *Resource.java Spring RestControllers (the plugin generates only the *Api, *Controller, and *Delegate classes and interfaces) this work should be provided in the documentation for completeness.

Furthermore, the following should be noted:

  1. the openapi-generator-maven-plugin should be configured to add DTO as a suffix to the models OR the dtoSuffix false set in application:config (pick one).
  2. the current generator-jhipster template will need some work; specifically in the openapi-generator-maven-plugin configuration, the properties set for the modelNameSuffix to DTO and change the modelPackage path to match the JHipster expectation when using use mapstruct ...

I'll get a demo project set up soon with the new PR.

@timothystone-knsl
Copy link
Contributor Author

I haven't abandoned this PR. I think this PR is still valuable, but I think the topic needs a larger rewrite with support in JHipster evaluated (see jhipster/generator-jhipster#27215).

It's been approved. Just needs a merge?

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.

API First Development Page Guide makes assumptions to work
4 participants