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

[KOGITO-9861] Fixing compilation issues with nested classes and arrays #3242

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Oct 3, 2023

Fix a problem that prevents using arrays as open api body request (top level, inner ones were already supported)
Also fixes compilation issues when open api extension generate pojos as inner classes

@fjtirado fjtirado changed the title [KOGITO-9861] Fixing compilation issues with nested and arrays [KOGITO-9861] Fixing compilation issues with nested classes and arrays Oct 4, 2023
@fjtirado fjtirado marked this pull request as ready for review October 4, 2023 12:57
@@ -48,10 +47,13 @@ public void executeWorkItem(KogitoWorkItem workItem, KogitoWorkItemManager manag
protected static <V> V buildBody(Map<String, Object> params, Class<V> clazz) {
for (Object obj : params.values()) {
if (obj != null && clazz.isAssignableFrom(obj.getClass())) {
logger.trace("Invoking workitemhandler with value {}", obj);
Copy link
Contributor Author

@fjtirado fjtirado Oct 4, 2023

Choose a reason for hiding this comment

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

This change is not stricly needed for this PR, but I feel it isinteresting to have a trace here.

return clazz.cast(obj);
}
}
return ObjectMapperFactory.get().convertValue(params, clazz);
V value = JsonObjectUtils.convertValue(params, clazz);
logger.trace("Invoking workitemhandler with value {}", value);
Copy link
Contributor Author

@fjtirado fjtirado Oct 4, 2023

Choose a reason for hiding this comment

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

Same here. Also, in some circumstances, JsonObjectUtils.converValue will be faster than directly invoking Jackson converter


private String fromDotName(DotName dotName) {
String result = dotName.toString();
if (dotName.isInner()) {
Copy link
Contributor Author

@fjtirado fjtirado Oct 4, 2023

Choose a reason for hiding this comment

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

Supporting inner classes, it is really unfortunate that DotName.toString use Class.getName rather than Class.getCanonicalName, forcing us to add this code to replace the $

return parseClassOrInterfaceType(param.asParameterizedType().name().toString())
.setTypeArguments(NodeList.nodeList(param.asParameterizedType().arguments().stream().map(this::fromClass).collect(Collectors.toList())));
ClassOrInterfaceType result = parseClassOrInterfaceType(fromDotName(param.asParameterizedType().name()));
if (includeGeneric) {
Copy link
Contributor Author

@fjtirado fjtirado Oct 4, 2023

Choose a reason for hiding this comment

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

And this is basically the fix (rest of clasess on this PR are for testing), we do not want to include generic when the ClassExpr is not used for casting. Since the caller knows the usage, an extra boolean parameter was added to the method

Copy link
Contributor

@tiagodolphine tiagodolphine left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@gabriel-farache
Copy link
Contributor

gabriel-farache commented Oct 4, 2023

Thank you for the quick fix :) 🐎

@fjtirado fjtirado merged commit 6bd9b68 into apache:main Oct 6, 2023
1 of 6 checks passed
rgdoliveira pushed a commit to kiegroup/kogito-runtimes that referenced this pull request Dec 13, 2023
apache#3242)

* [KOGITO-9861] Fixing compilation issues with nested and arrays

* [KOGITO-9861] Adding IT test
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.

None yet

4 participants