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

PAYARA-4202 PAYARA-4206 Support for optional arrays and char type in config #4262

Merged
merged 5 commits into from Oct 9, 2019

Conversation

@pdudits
Copy link
Contributor

pdudits commented Oct 8, 2019

Description

This is a bug fix.

Obtaining an array over Config.getOptionalValue(String, Class) previously threw an exception

Caused by: java.lang.IllegalArgumentException: Unable to convert value to type java.lang.String[]
        at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getConverter(PayaraConfig.java:243)
        at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getOptionalValue(PayaraConfig.java:99)

First commit of the PR fixes this.

This use of API was not covered by TCK test, therefore tests have been added upstream in eclipse/microprofile-config#452.

Running against these tests showed, that we miss converters for char and Character types, which is added by the second commit.

Last commit cleans up logic around selecting fitting converter for a type, and moves discovery of automatic converters into CommonSenseConverter.forClass(Class)

Important Info

Testing

New tests

eclipse/microprofile-config#452

Testing Performed

Test suites executed

  • Payara Microprofile TCKs Runner - Config

Testing Environment

Zulu JDK 1.8_221 on Windows 10 with Maven 3.6.1

Notes for Reviewers

pdudits added 3 commits Oct 8, 2019
Converters were looked up before tokenization in convertString was applied
Logic moved into the class itself, flow in PayaraConfig was made more linear
@pdudits pdudits force-pushed the pdudits:payara-3828 branch from 1c8b5cb to bec62c7 Oct 8, 2019
@pdudits

This comment has been minimized.

Copy link
Contributor Author

pdudits commented Oct 8, 2019

jenkins test please

@pdudits pdudits requested a review from MattGill98 Oct 8, 2019
private static <T> Class<T> boxedTypeOf(Class<T> type) {
if (!type.isPrimitive()) {
return type;
} else if (type.equals(int.class)) {

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

else can be spared - I know it makes the method longer with our formatting rules but it is simpler without the else. equals can be ==.

I benchmarked this years ago - using a map is surprisingly equally fast but I also ended up keeping the if's since those can be ordered checking for the most likely first.

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 8, 2019

Author Contributor

Since this ends up just being key in the map, I think I'll just initialize default converters map with both types.

}
return converter;
return type;

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

We already established that type is a primitive type so should we ever get here this should mean a primitive had been added to the JVM and java that we have not covered so its more of a throw new situation if you ask me.

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 8, 2019

Author Contributor

Yes, but do we want such surprise few years later? :D But yes, initially these branches didn't list short and char.

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

We could also change the name to boxedTypeOrNot to make the caller aware of this behaviour :D

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 9, 2019

Author Contributor

Actually, the only type reaching that return statement is... void, which is quite improbable for config property, but makes the code completely correct :D

break;
}
// search for a matching raw type
for (Entry<Type, Converter> entry : converters.entrySet()) {

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

Maybe the converters should be organized by raw type first, then in a map of parameterized types as second level.

if (type1 instanceof ParameterizedType) {
ParameterizedType ptype = (ParameterizedType)type1;
if (ptype.getRawType().equals(propertyType)) {
return entry.getValue();

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

Returning the first converter with matching raw types must be a bug. Say I have a converter List<String> and List<? extends Number> this could respond with List<String> when I ask for List<Integer> even though there was a proper match.

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 8, 2019

Author Contributor

That's right, I think the support for generic types work only for Lists and Sets, and it has specific code in CDI extension for that. I'll remove this part


/**
*
* @author steve
*/
public class CommonSenseConverter implements Converter<Object> {

/**
* Return implicit converter for a class following section "Automatic Converters" of the spec, if class has

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

Just for the record: I'd find the name AutomaticConverter as suggested by this comment much more explanatory.




private static <T> Class<T> boxedTypeOf(Class<T> type) {

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

The spec says (https://github.com/eclipse/microprofile-config/blob/master/spec/src/main/asciidoc/converters.asciidoc):

The following Converters are provided by MicroProfile-Config by default:

Then naming primitive and their wrapper types. Does this mean this code is actually build into some shared code or does it want to tell us how we have to implement this? If this is already implemented there should not be a need to have this method since primitives and their wrappers are already covered.

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 8, 2019

Author Contributor

The latter. We need to implement this, which is exactly why this conversion happens. Other option would be to populate default converters map with both options, which is likely even simpler.

This comment has been minimized.

Copy link
@jbee

jbee Oct 8, 2019

Contributor

Unless... overrides. And of course there can be.

@MarkWareham

This comment has been minimized.

Copy link
Contributor

MarkWareham commented Oct 8, 2019

jenkins test please

1 similar comment
@MarkWareham

This comment has been minimized.

Copy link
Contributor

MarkWareham commented Oct 8, 2019

jenkins test please

- CommonSenseConverter renamed to AutomaticConverter to match the spec.
- Simplified flow of boxedTypeOf
- Converterers are looked up by type rather than class to allow support for full generic types later.
@pdudits pdudits requested a review from jbee Oct 9, 2019

/**
*
* @author steve
*/
public class CommonSenseConverter implements Converter<Object> {
public class AutomaticConverter implements Converter<Object> {

This comment has been minimized.

Copy link
@jbee

jbee Oct 9, 2019

Contributor

Needs copyright update ;(

@@ -39,29 +39,24 @@
*/
package fish.payara.nucleus.microprofile.config.spi;

import fish.payara.nucleus.microprofile.config.converters.CommonSenseConverter;
import fish.payara.nucleus.microprofile.config.converters.AutomaticConverter;

This comment has been minimized.

Copy link
@jbee

jbee Oct 9, 2019

Contributor

Needs copyright update.

@@ -62,6 +62,7 @@
import javax.inject.Inject;
import javax.inject.Named;

import fish.payara.nucleus.microprofile.config.converters.CharacterConverter;

This comment has been minimized.

Copy link
@jbee

jbee Oct 9, 2019

Contributor

Needs copyright update.

@jbee
jbee approved these changes Oct 9, 2019
Copy link
Contributor

jbee left a comment

I got my name change 🎆
Just needs some copyright updates.

@pdudits

This comment has been minimized.

Copy link
Contributor Author

pdudits commented Oct 9, 2019

jenkins test please

@pdudits pdudits merged commit 33db41d into payara:master Oct 9, 2019
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.