Skip to content

Added support for multipart form data in OpenAPI-to-JAX-RS generator#391

Merged
EricWittmann merged 4 commits intoApicurio:mainfrom
lpieprzyk:main
Mar 3, 2026
Merged

Added support for multipart form data in OpenAPI-to-JAX-RS generator#391
EricWittmann merged 4 commits intoApicurio:mainfrom
lpieprzyk:main

Conversation

@lpieprzyk
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +493 to +506
// Try to extract method name from path segment
if (currentPath != null && currentPath.length() > 0) {
String[] pathSegments = currentPath.split("/");
if (pathSegments.length > 0) {
String lastSegment = pathSegments[pathSegments.length - 1];
if (lastSegment != null && lastSegment.trim().length() > 0) {
// Remove path parameters (e.g., {id}) and sanitize
String sanitized = lastSegment.replaceAll("\\{[^}]*\\}", "").replaceAll("\\W", "");
if (sanitized.length() > 0) {
return sanitized;
}
}
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@EricWittmann is this okay here? I needed to add this, because the generated method was unexpectedly named "generatedMethod1". I also edited the chines character test accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine to add it here, but maybe we need to check for conflicts with names generated the other way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I missed something, but all tests still run. Is there a specific test which covers the name generated the other way?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most of the tests cover the name generated from the operationId - but if there is an operationId with the same value as what is produced by this new code path, then we would have a conflict. I guess we would need a new test for that - basically an OpenAPI with an Operation that has an operationId that is the same as another operation without an operationId that will get resolved using this new approach.

Copy link
Copy Markdown
Member

@EricWittmann EricWittmann left a comment

Choose a reason for hiding this comment

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

I love this! You've done a lot of work here. I would need to dig a lot more into this space to really give this a proper review, but it looks pretty good overall. I have some comments/requests for changes. In addition to those, here is some high level feedback.

The biggest issue is that it's fully coupled to OpenAPI 3.1. Unless we are auto-converting the spec to OpenAPI 3.1 before running this code, we'll need to support both 3.0 and 3.1. Are we auto-converting to 3.1?! We might be...

Maybe some error test cases would be useful:
- Invalid schema types
- Missing required fields
- Null schemas
- Non-multipart content types
- Mixed content types (multipart + json)

Comment on lines +493 to +506
// Try to extract method name from path segment
if (currentPath != null && currentPath.length() > 0) {
String[] pathSegments = currentPath.split("/");
if (pathSegments.length > 0) {
String lastSegment = pathSegments[pathSegments.length - 1];
if (lastSegment != null && lastSegment.trim().length() > 0) {
// Remove path parameters (e.g., {id}) and sanitize
String sanitized = lastSegment.replaceAll("\\{[^}]*\\}", "").replaceAll("\\W", "");
if (sanitized.length() > 0) {
return sanitized;
}
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine to add it here, but maybe we need to check for conflicts with names generated the other way?

@EricWittmann
Copy link
Copy Markdown
Member

Hey @lpieprzyk - I was out on vacation last week. What's the status of this PR? Ready to go? All requested changes applied? Or not yet?

@lpieprzyk
Copy link
Copy Markdown
Contributor Author

Hey @lpieprzyk - I was out on vacation last week. What's the status of this PR? Ready to go? All requested changes applied? Or not yet?

I still have a small bug I’m working on, but I hope to be done by the end of the week.

@EricWittmann
Copy link
Copy Markdown
Member

Cool no worries. I'm not pushing you - I just wanted to make sure you weren't waiting on me. :)

Comment on lines +221 to +244
/**
* Checks for duplicate method names within the current interface and throws a
* {@link IllegalStateException} if a conflict is detected. This can occur when multiple
* operations resolve to the same method name, typically due to missing or identical
* {@code operationId} values in the OpenAPI specification.
*
* @param method the new method to check against existing methods in the current interface
* @throws IllegalStateException if a method with the same name already exists in the interface
*/
private void checkForDuplicateMethodNames(CodegenJavaMethod method) {
String newMethodName = method.getName();
for (CodegenJavaMethod existingMethod : this._currentInterface.getMethods()) {
if (existingMethod.getName().equals(newMethodName)) {
throw new IllegalStateException(
"Duplicate method name '" + newMethodName + "' detected in interface '"
+ this._currentInterface.getName() + "'. "
+ "Operation at path '" + currentPath + "' (HTTP " + method.getMethod()
+ ") resolves to the same method name as an existing operation. "
+ "Consider adding a unique 'operationId' to one of the conflicting operations."
);
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@EricWittmann Is this okay with you? In my opinion it is a configuration error in the spec file, if there are two endpoints with the same name. Or should I generate the endpoints with a counter suffix?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@lpieprzyk
Copy link
Copy Markdown
Contributor Author

@EricWittmann I added a testcase for a 3.0.0 Spec. And it seems like we are auto-converting :D
I also added test cases for the edge cases you listed. Are the cases handled as you expected or do they need changes?

@EricWittmann
Copy link
Copy Markdown
Member

Test cases look good to me! 👍 Let me know if/when you think it's ready to merge and I'll take a final look before merging.

@lpieprzyk
Copy link
Copy Markdown
Contributor Author

I think all requested changes (if I didn´t miss anything) are done.

@EricWittmann EricWittmann merged commit aba7c61 into Apicurio:main Mar 3, 2026
2 checks passed
@EricWittmann
Copy link
Copy Markdown
Member

Merged! 🎉

@mcruzdev
Copy link
Copy Markdown
Contributor

mcruzdev commented Mar 5, 2026

Hi @EricWittmann, could you generate a new release?

@EricWittmann
Copy link
Copy Markdown
Member

In progress.

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