Skip to content

Ensure correct ordering of delegates regardless of user input #3956

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@

package org.springframework.kafka.support.serializer;

import java.util.LinkedHashMap;
import java.util.Map;
import java.util.*;
import java.util.Map.Entry;

import org.apache.kafka.common.errors.SerializationException;
Expand All @@ -32,13 +31,27 @@
* @author Gary Russell
* @author Artem Bilan
* @author Wang Zhiyang
* @author Mahesh Aravind V
*
* @since 2.7.9
*
*/
public class DelegatingByTypeSerializer implements Serializer<Object> {

private final Map<Class<?>, Serializer<?>> delegates = new LinkedHashMap<>();
private static final Comparator<Class<?>> DELEGATES_ASSIGNABILITY_COMPARATOR =
(type1, type2) -> {

if (type1.isAssignableFrom(type2)) {
return 1;
}
if (type2.isAssignableFrom(type1)) {
return -1;
}

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if more robust algorithm would be like this:

public class ClassHierarchyComparator implements Comparator<Class<?>> {

    @Override
    public int compare(Class<?> class1, Class<?> class2) {
        if (class1 == class2) {
            return 0; // Classes are the same
        }
        if (class1.isAssignableFrom(class2)) {
            return -1; // class1 is a superclass or superinterface of class2, so class1 should come first
        }
        if (class2.isAssignableFrom(class1)) {
            return 1; // class2 is a superclass or superinterface of class1, so class2 should come first
        }
        return class1.getName().compareTo(class2.getName()); // If no inheritance relation, compare by name
    }
}

};

private final Map<Class<?>, Serializer<?>> delegates = new TreeMap<>(DELEGATES_ASSIGNABILITY_COMPARATOR);

private final boolean assignable;

Expand All @@ -51,17 +64,23 @@ public DelegatingByTypeSerializer(Map<Class<?>, Serializer<?>> delegates) {
}

/**
* Construct an instance with the map of delegates; keys matched exactly or if the
* target object is assignable to the key, depending on the assignable argument.
* If assignable, entries are checked in the natural entry order so an ordered map
* such as a {@link LinkedHashMap} is recommended.
* @param delegates the delegates.
* @param assignable whether the target is assignable to the key.
* Construct an instance with the map of delegates.
* If {@code assignable} is {@code false}, only exact key matches are considered.
* If {@code assignable} is {@code true}, a delegate is selected if its key class
* is assignable from the target object's class. When multiple matches are possible,
* the most specific matching class is selected — that is, the closest match in the
* class hierarchy.
*
Copy link
Member

Choose a reason for hiding this comment

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

No blank lines in method JavaDocs, please.
You can use spring-framework.xml IntelliJ IDEA editor config from the project to realign with our code style.

Copy link
Member

Choose a reason for hiding this comment

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

Have not been addressed yet

* @param delegates the delegates
* @param assignable true if {@link #findDelegate(Object, Map)} should consider assignability to
* the key rather than an exact match.
*
* @since 2.8.3
*/
public DelegatingByTypeSerializer(Map<Class<?>, Serializer<?>> delegates, boolean assignable) {
Assert.notNull(delegates, "'delegates' cannot be null");
Assert.noNullElements(delegates.values(), "Serializers in delegates map cannot be null");

this.delegates.putAll(delegates);
this.assignable = assignable;
}
Expand Down Expand Up @@ -101,7 +120,7 @@ public byte[] serialize(String topic, Headers headers, Object data) {
return delegate.serialize(topic, headers, data);
}

private <T> Serializer<T> findDelegate(T data) {
protected <T> Serializer<T> findDelegate(T data) {
return findDelegate(data, this.delegates);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2021-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.kafka.support.serializer;

import java.util.LinkedHashMap;
import java.util.Map;

import org.apache.kafka.common.serialization.Serializer;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

/**
* @author Mahesh Aravind V
*
*/
class DelegatingByTypeSerializerTests {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you create a new test class if we have one on the matter already: DelegatingSerializationTests?
Please, consider to add your test over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a class DelegatingSerializer and the DelegatingSerializationTests is for tests of that class I assume.

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 true, but I see tests for DelegatingByTypeSerializer over there, so I would like to have your new one over there as well.
And what is the reason of @Nested?
Cannot we really have it as a top-level test method and have the code flow as simple as it could be?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaheshAravindV It's better to have all the tests in DelegatingSerializationTests. Could you move it there?

@Nested
class AssignableTest {
@Test
void shouldOrderDelegatesSoChildComesBeforeParent() {
class Parent { }

class Child extends Parent { }

Serializer mockParentSerializer = mock(Serializer.class);
Serializer mockChildSerializer = mock(Serializer.class);

// Using LinkedHashMap to ensure the order is always wrong
Map<Class<?>, Serializer<?>> delegates = new LinkedHashMap<>();
delegates.put(Parent.class, mockParentSerializer);
delegates.put(Child.class, mockChildSerializer);

DelegatingByTypeSerializer serializer = new DelegatingByTypeSerializer(delegates, true);


Serializer childSerializer = serializer.findDelegate(mock(Child.class));
Serializer parentSerializer = serializer.findDelegate(mock(Parent.class));


assertThat(childSerializer).isEqualTo(mockChildSerializer);
assertThat(parentSerializer).isEqualTo(mockParentSerializer);
}
}
}