CompositeCodec constructors require at least one delegate

[cppwfs]

• In the current implementation, users could pass in an empty list to the composite codec.
• This is an CompositeCodec and if we had no codecs, then why have the class.
• All Nullable's were removed from the delegate attribute.
• Null returns from the findDelegate have been removed. The default delegate will be returned in the stead.
• Update the tests to require at least one codec in the delegates

[artembilan]

Please, add your name to the @author list of all the affected classes.

[artembilan]

I know, this is not the part of the problem we are fixing here, but I’d like to question this part. What does it mean and if it is explained in the JavaDocs and covered by tests.
Plus why there is no null check for this method call? Does it fail with error and that is what that your new code in the test? How does fallback to the default work then ?
Would you mind looking into that, please ?

[mrpiggi]

I just saw, that I started a review on the former related PR but did not submit it so here my thoughts from the last review:

As ClassUtils.findClosestMatch() can return null (through failOnTie=false), one could argue that Map::get may not be called with null as key (which is no concern with HashMap but can be harmful if implementation is changed. So a safe approach would be

private Codec findDelegate(Class<?> type) { Class<?> clazz = ClassUtils.findClosestMatch(type, this.delegates.keySet(), false); return clazz == null ? this.defaultCodec : this.delegates.getOrDefault(clazz, this.defaultCodec);	}

Regarding the question from Artem:

ClassUtils.findClosestMatch() traverses the class hierarchy by calling ClassUtils.getTypeDifferenceWeight() and gives back the class itself if found in this.delegates.keySet(). Otherwise, the "closest" super class or interface in the hierarchy found is returned.

As failOnTie=false is set, there will be something returned if a match in this.delegates was found. But as there could be multiple interfaces on the same "hierarchy level" in this.delegates and ClassUtils.findClosestMatch() is called with an unordered set, in this case the returned interface is non-deterministic. So one could argue that setting failOnTie=true would be the more defensive and reliable approach, though I have no suggestion, how one should then deal with the potentially thrown IllegalStateException

[cppwfs]

Argue with me please. We could give that option to the user and they make the decision. But we still support our original spec from the Codec interface:

/** * Finds the closest matching codec from the given set of candidate codecs based on the * type difference weight between the specified target type and each candidate. * Then encodes an object to a byte array using the chosen codec. * @param object the object to encode * @param failOnTie whether to fail with an exception if two codecs have equal minimal weight * @return the bytes * @throws IOException if the operation fails */ public byte[] encode(Object object, boolean failOnTie) throws IOException { Assert.notNull(object, "cannot encode a null object"); return findDelegate(object.getClass(), findOnTie).encode(object); } /** * Finds the closest matching codec from the given set of candidate codecs based on the * type difference weight between the specified target type and each candidate. * Then encodes an object to a byte array using the chosen codec. If 2 codes have the same weight, then one of the two will be chosen. * @param object the object to encode * @return the bytes * @throws IOException if the operation fails */ public byte[] encode(Object object) throws IOException { Assert.notNull(object, "cannot encode a null object"); return findDelegate(object.getClass(), false).encode(object); }

Thoughts?

[artembilan]

Thank you for looking into this!

I had a chance today as well:

else if (failOnTie && typeDiffWeight < Integer.MAX_VALUE && (typeDiffWeight == minTypeDiffWeight)) {	throw new IllegalStateException("Unresolvable ambiguity while attempting to find closest match for [" +	type.getName() + "]. Candidate types [" + closestMatch.getName() + "] and [" +	candidate.getName() + "] have equal weight.");	} 

And I think this is the best approach we could live so far.
There is really no guarantee which one would be returned and different types may lead to different delegates.
And in different environments (or different application runs) we could end up with different delegates.
Therefore the best approach is to failOnTie and let the target project to fix configuration for this component to avoid ambiguity after seeing that IllegalStateException.

I want that to be mentioned in class JavaDocs.

Nit-pick: the method JavaDoc has to be imperative (like a command), not infinitive 😉

[mrpiggi]

See my discussion with Artem, this will fail if the deprecated ctor is used

[mrpiggi]

Instead of calling the main ctor, one could go with

@Deprecated(since = "7.0", forRemoval = true) public CompositeCodec(Codec defaultCodec) {	this.defaultCodec = defaultCodec;	this.delegates = Map.of(); } 

[artembilan]

I don't think this is what I said in the discussion.
The current (release) CompositeCodec for this ctor is to fail with NPE:

public CompositeCodec(Map<Class<?>, Codec> delegates, Codec defaultCodec) {	Assert.notNull(defaultCodec, "'defaultCodec' cannot be null");	this.defaultCodec = defaultCodec;	this.delegates = new HashMap<Class<?>, Codec>(delegates);	}	public CompositeCodec(Codec defaultCodec) {	this(null, defaultCodec);	} 

That HashMap(Map<? extends K, ? extends V> m) does not let to pass a null.
So, with our new Assert.notEmpty(Map), there is no too much difference with current behavior.
We still fail, but with extra deprecation warning.

Why you have decided to change behavior?

[mrpiggi]

That was my suggestion, but I had no look in the 6.5.x branch so far. You are totally right, new HashMap<>(null) obviously cannot work. But then, this should be considered as a (10 year old) flawed/buggy implementation. And I still think, there are only two reasonable choices:

a) deprecate the ctor but provide a sane implementation
b) remove the flawed ctor completely without deprecation as this has never worked in the first place

I don't see, why an obvious bug should be kept although it was identified. I don't see any benefit from providing a flawed implementation and would rather prefer an error on compile time. But the call is up to you.

[artembilan]

Yes. This is great thinking.
I totally missed the fact that we can deprecate this ctor in previous versions and remove it altogether here in 7.0.

I see your point about reasoning now.
So, let's make it with an empty map in 6.5.x & 6.4.x and mark as deprecated for removal over there.
Then we have a freedom here in main to remove it at all.

Can we go this route?

[mrpiggi]

I totally missed the fact that we can deprecate this ctor in previous versions and remove it altogether here in 7.0.

Great idea. I missed that as well ;)

Can we go this route?

I totally agree!

[artembilan]

This ha not be addressed.
I mean saying messageCodec.getClass() is wrong from the context of delegates logic.

I think this test could be fixed like this:

this.codec = new MessageCodec(); 

As we realized for the goal of this PR.
Why would one use a Composite if there is no composition?