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

Add moqu and wiremock module #826

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcruzdev
Copy link
Member

@mcruzdev mcruzdev commented Oct 23, 2024

Why

This pull request aims to add a new module where we get a OpenAPI Specification and transform it to a Wiremock (this pull request, we can have more after) stubs.

This was created from this Discussion: #575

DevUI

Screen.Recording.2024-11-20.at.13.18.08.mov

@melloware
Copy link
Contributor

This is interesting but i don't see any Doc updates explaining what this exactly does?

@mcruzdev
Copy link
Member Author

Hi @melloware, yeah I need to add documentation ;/ I will try to do it on this week.

Copy link
Contributor

@melloware melloware left a comment

Choose a reason for hiding this comment

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

awesome

@mcruzdev mcruzdev marked this pull request as ready for review November 20, 2024 16:36
@mcruzdev mcruzdev requested a review from a team as a code owner November 20, 2024 16:36
@mcruzdev
Copy link
Member Author

mcruzdev commented Nov 20, 2024

Could you add your review too? @ricardozanini @hbelmiro @rmanibus @fjtirado

There are some things to do yet, but we can do in another step.

  • Generate wiremock-mappings.json on output directory.
  • Add a DevService for hosting the Wiremock for test/dev purpose (maybe to integrate with quarkisver-wiremock)

@mcruzdev
Copy link
Member Author

cc: @chberger

@ricardozanini
Copy link
Member

Do we have an issue explaining the motivations of this PR?

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

That seems a huge amount of work, thank you!

I won't micro review it, I'd rather wait for community's feedback and traction. My only concern is about this addition to this ecosystem, a comprehensive documentation and README updates to differentiate between the modules. Can we work on these?

Why can't we have just the three sets of projects:

moqu
-- core
-- deployment
-- runtime

I don't the need to split the extension into yet another parent POM.

@mcruzdev
Copy link
Member Author

Do we have an issue explaining the motivations of this PR?

This is related to this discussion #575

@fjtirado fjtirado self-requested a review November 22, 2024 11:41
Copy link
Contributor

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

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

There are some leftovers that requires cleaning before merging.

Comment on lines +10 to +17
private static ObjectMapper objectMapper;

public static ObjectMapper getInstance() {
if (objectMapper == null) {
objectMapper = new ObjectMapper();
}
return objectMapper;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static ObjectMapper objectMapper;
public static ObjectMapper getInstance() {
if (objectMapper == null) {
objectMapper = new ObjectMapper();
}
return objectMapper;
}
private static final ObjectMapper objectMapper = new ObjectMapper();
public static ObjectMapper getInstance() {
return objectMapper;
}

* @param name the name of the HTTP header (e.g., "Accept", "Content-Type").
* @param value the set of values associated with the header, allowing multiple values (e.g., "application/json", "text/html").
*/
public record Header(String name, Set<String> value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public record Header(String name, Set<String> value) {
public record Header(String name, List<String> value) {

Although duplicating string values for a header seems weird, it can actually make sense in some scenarios. Also, header values are an ordered collection, List should be used rather than Set

* @param accept the "Accept" header, which specifies the expected response format.
* @param parameters the list of parameters to be included in the request.
*/
public record Request(String url, String httpMethod, String exampleName, Header accept, List<Parameter> parameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public record Request(String url, String httpMethod, String exampleName, Header accept, List<Parameter> parameters) {
public record Request(String url, String httpMethod, String exampleName, Header accept, Collection<Parameter> parameters) {

Here is the other way around, Paremeters cannot be duplicated in an http request

import java.nio.file.Files;
import java.nio.file.Path;

public class Testing {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is a left over, I guess you want to delete it

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.

4 participants