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

[599] Add better support for resolution of wildcard types. #661

Merged
merged 3 commits into from
Mar 13, 2025

Conversation

jamezp
Copy link
Contributor

@jamezp jamezp commented Feb 6, 2025

This should resolve the following:

Overall, this should improve the way some of the wildcard types are resolved. The fix in ReflectionUtils was for an issue filed against JBoss EAP. If we need another issue here for this, please let me know. However, it seemed the two above may cover it.

@jamezp jamezp force-pushed the issue599 branch 2 times, most recently from 332fada to 72781a6 Compare February 6, 2025 01:57
@@ -233,7 +233,7 @@ private ModelDeserializer<JsonParser> createObjectDeserializer(LinkedList<Type>
if (creatorModel.getCustomization().isRequired()) {
defaultCreatorValues.put(parameterName, new RequiredCreatorParameter(parameterName));
} else {
Class<?> rawParamType = ReflectionUtils.getRawType(creatorModel.getType());
Class<?> rawParamType = ReflectionUtils.getOptionalRawType(creatorModel.getType()).orElse(Object.class);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure if this is the correct place to fix this. I did attempt to fix it by defaulting to Optional.of(Object.class) in the ReflecionUtils.getOptionalRawType(). However, that did break some tests and I wasn't sure if that was correct either.

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2023 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

According to Eclipse Foundation, when omitting the second date, you must keep the remaining one unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. I had it backwards. I'll fix that to simply remove the second date and keep the first date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that fails the copyright check. I'm not sure what the right approach is.

Copy link
Member

Choose a reason for hiding this comment

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

This means, the copyright check is not compliant to the rules set up by the Eclipse Foundation, simply spoken. Things like this often happens if POMs were copied (or authored) from Oracle.

Choose a reason for hiding this comment

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

@mkarg does the code change for the fix look ok? I have a user hitting this as a regression when moving from yasson2 to yasson3 , Thanks !

Copy link
Member

Choose a reason for hiding this comment

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

It is up to the Yasson committers to decide.

Copy link
Member

Choose a reason for hiding this comment

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

@m0mus According to Eclipse Foundation's rules, all projects MUST accept the short form having only the initial publication date (see https://www.eclipse.org/projects/handbook/#ip-copyright-headers). Apparently Yasson's POM.xml expects to find the latest date, which is wrong. What is your decision how to proceed?

@jamezp
Copy link
Contributor Author

jamezp commented Feb 17, 2025

I've added an additional commit to not check the year when validating the copyright.

@jamezp
Copy link
Contributor Author

jamezp commented Mar 5, 2025

@m0mus @Verdent Could one of you look at this by chance? We have a customer waiting for a fix for JBoss EAP and I'd like to avoid forking if possible. Thanks in advance.

@Verdent
Copy link
Member

Verdent commented Mar 13, 2025

Hi @jamezp , sorry for the delay. I will take a look :-)

@Verdent Verdent merged commit 5d6af1a into eclipse-ee4j:master Mar 13, 2025
4 checks passed
@jamezp
Copy link
Contributor Author

jamezp commented Mar 13, 2025

Awesome, thank you @Verdent! I know it's a big ask, but is there a release schedule for this by chance? I'm happy to run some TCK's against WildFly and a SNAPSHOT of this first if that helps.

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