XBEAN-453 - honor Option.CASE_INSENSITIVE_PROPERTIES for ctors #47
Conversation
| Class type = parameterTypes.get(i); | ||
| if (i > 0) buffer.append(", "); | ||
| buffer.append(type.getName()); | ||
| buffer.append(type != null ? type.getName() : "..."); |
There was a problem hiding this comment.
tomee passes parameterTypes=null which is then converted to a list of nulls in findConstructor, and toParameterList gets invoked to generate exception messages
There was a problem hiding this comment.
Hmm, I guessed so but why does it happen now and was not before? Is it about the ServiceProviders, did something changed ?
to be clear I'm not against the change just want to ensure it is not a misusage cause we are loosing information there in user facing error messages, will be less usable ("oh it failed in constructor Foo(..., ..., ....)")
There was a problem hiding this comment.
I think this happened as a side effect of XBEAN-350. Since we fixed that issue, the code path is now reachable with null type parameters through implicit constructor resolution using argument names (in which case the type list is nulled). On the TomEE side, nothing has changed in that regard
There was a problem hiding this comment.
Agree, didn't get a change to check but was my fear, so this sounds like a regression we might want to fix before next release - and therefore not do this change in this PR, wdyt?
There was a problem hiding this comment.
I don't think it's a regression technically. More or less a bug of this utility method to produce a user facing error messages and don't account for null values.
What we might want to fix is likely another type of error message accounting for the null containing type list in case we fail via implicit ctor arg resolution and no types are available/present. That would be more helpful for the error msg.
I agree - we can change and track it in an other issue and revert it here.
There was a problem hiding this comment.
Proposed to use Object instead of ... to explicit it was a loose matching -> #48, feel free to take it back or rework it, it is just a proposal
| } | ||
|
|
||
| boolean parametersMatch; | ||
| if (options.contains(Option.CASE_INSENSITIVE_PROPERTIES)) { |
| String name = parameterNames.get(i); | ||
|
|
||
| Property property = null; | ||
| if (options.contains(Option.CASE_INSENSITIVE_PROPERTIES)) { |
There was a problem hiding this comment.
what about testing if the map is case insensitive and if not just copy it in a treemap or alike (or do it in the caller)?
There was a problem hiding this comment.
tbh i wasnt aware treemap/treeset could to that, updated my PR and it makes the diff a lot smaller
| // clone the properties so they can be used again | ||
| Map<Property,Object> propertyValues = new LinkedHashMap<Property,Object>(properties); | ||
| Map<Property,Object> propertyValues = options.contains(Option.CASE_INSENSITIVE_PROPERTIES) | ||
| ? new TreeMap<>(Comparator.comparing(property -> property.name, String.CASE_INSENSITIVE_ORDER)) |
There was a problem hiding this comment.
new TreeMap<>(String.CASE_INSENSITIVE_ORDER) should be sufficient
rmannibucau
left a comment
rmannibucau
left a comment
There was a problem hiding this comment.
Overall ok, I'd simplify the treemap thing and review why tomee fails but core change looks good
3 participants
As the title says, see https://issues.apache.org/jira/browse/XBEAN-353