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

API First Development Page Guide makes assumptions to work #25030

Closed
1 task done
timothystone-knsl opened this issue Jan 31, 2024 · 10 comments · Fixed by jhipster/jhipster.github.io#1335
Closed
1 task done

Comments

@timothystone-knsl
Copy link
Contributor

timothystone-knsl commented Jan 31, 2024

Overview of the issue

The Doing API-First development page has some fundamental assumptions and will not build due to ArchUnit if you follow the guide filling in the gaps where assumptions of understanding are made.

Motivation for or Use Case

The Doing API-First development SHOULD be an explicit guide for new comers to JHipster and to API First Development.

Reproduce the error

Follow the guide explicitly. Where you find yourself making an assumption about JHipster, stop, document it, and put it in the guide.

Related issues
Suggest a Fix

The guide works up to the part of generate-sources that quickly begins describing implementation details that make assumptions that the user approaching the guide cannot divine without prior experience with all of the concepts being presented.

The example shows an implementation of the delegates, that when logically placed in a JHipster app in the service directory (the example doesn't provide a package sample, so observation leads to placing the @Service class in the service directory). But the build fails because the ArchUnit test finds the effort using the "guide" in violation. This suggests the guide was written prior to the introduction of ArchUnit, though I've never known JHipster without it.

I'm willing to take on some of this work and will be looking into it so watch for an PR.

It's clear that JHipster has come down on one side of the debate, to Delegate or Not to Delegate. OpenAPI Generator defaults Not to Delegate. This is also a missing element of the documentation. I think decisions like that should be explicitly documented so that users can make choices on changing the pattern in the generated code.

See PR jhipster/jhipster.github.io#1335

JHipster Version(s)

8.1.0

JHipster configuration

Not Applicable

Browsers and Operating System

Not Applicable

  • Checking this box is mandatory (this is just to show you read everything)
@timothystone-knsl
Copy link
Contributor Author

I see that @atomfrede seems to have acknowledged the problem in #10552 but the documentation never got updated 🤔 💭

@atomfrede
Copy link
Member

Thanks @timothystone-knsl for bringing this up. Indeed it seems the docs are rather short an lack some level of detail. Would you like to rework that part?

@timothystone-knsl
Copy link
Contributor Author

Yes, I'll be looking at adding the necessary detail, though I am not, by default, a Delegate evangelist, so learning here too. I will push a branch off a fork as time permits, soon.

@atomfrede
Copy link
Member

Thanks @timothystone-knsl . In case you think the current configuration with delegates is not optimal feel free to change it or lets discuss another proposal.

@timothystone-knsl
Copy link
Contributor Author

I don't have a problem with the Delegate pattern. It has its merits.

My issue is that if the pattern runs afoul of the Technical Structure tests enforced by JHipster's out of the box configuration of ArchUnit, well, something has to give. There seems to be some history there in #10552 that I don't have full context on.

I think that if all things held equal, the documentation should explicitly have notes on the following:

  1. How the openapi-generator-maven-plugin sets the delegatePattern to true
  2. What classes are generated
  3. What implementations are necessary
  4. Where the implementations should be placed in the application to meet the defined Technical Structure
  5. More complete samples showing the overridden methods that produce the "advertised" results*
  6. More liberal use of H1-6 for organization

* For example the documentation currently states:

If you provide the NativeWebRequest bean to the delegate interface, then basic example bodies will be returned for the methods that have not been overridden (still with a 501 HTTP status code). This is useful to mock your endpoints before providing the actual implementation.

This doesn't actually work in practice: the "basic example bodies" are not returned, only the 501. If this doesn't work as advertised in 8.1.0, it should not be in the documentation. Or the example should be updated to make it work (which I'm trying to do as part of my PR).

@atomfrede
Copy link
Member

atomfrede commented Feb 1, 2024

I have not used the open api generator recently (last time maybe 2 years ago) so I guess a lot has changed and as often documentation is not updated. I totally agree to have more clear examples with what is generated it helps a lot. And maybe some features have been removed upstream.

Edit: and yes I think the arch unit tests have been introduced after open api generator. So maybe they can/need to be adopted for open api

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Feb 4, 2024

@atomfrede et al., extensive rewrite of Doing API First Development page... review deployment here... https://deploy-preview-1335--jhipster-site.../ ... seems there's an infinite redirect. need to sort out how that happened.

There's nothing in the code that seems to be different than expected. The YAML frontmatter wasn't touched.

@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Feb 4, 2024

@atomfrede the redirect seems to be a problem in the Netlify deployment. It exists in ALL THREE (3) open PRs where the Doing API First Development page has not been altered. Someone with Netlify configuration authorization/authority should look at that with some degree of priority.

EDIT Matt Raible looked into the redirect issue. Appears to be a known issue with some pages. I'll attempt to run locally, but the page can be reviewed here, https://github.com/jhipster/jhipster.github.io/blob/2abcfc46a02874f47575f3f0e4076a40832c3c59/pages/doing_api_first_development.md without the netlify deployment.

@timothystone-knsl timothystone-knsl changed the title API First Development Page Guide has assumptions and doesn't work API First Development Page Guide makes assumptions to work Feb 4, 2024
@timothystone-knsl
Copy link
Contributor Author

timothystone-knsl commented Feb 6, 2024

Should feedback go to the PR or remain here @atomfrede?

@atomfrede
Copy link
Member

I would say lets do it directly in the pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants