Skip to content

Fix type confusion in Serializer.getSerializerRegistration #2604

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.

Sign up for GitHub

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

Open
Copilot wants to merge 2 commits into
base: master
Choose a base branch
from
+1 −1
Open

Fix type confusion in Serializer.getSerializerRegistration #2604

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28431d3
Initial plan
Copilot Feb 11, 2026
a1496fa
Fix: Use java.io.Serializable.class instead of Serializable.class on …
Copilot Feb 11, 2026
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
2 changes: 1 addition & 1 deletion jme3-networking/src/main/java/com/jme3/network/serializing/Serializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ public static SerializerRegistration getSerializerRegistration(Class cls, boolea

if (cls.isArray()) return registerClass(cls, arraySerializer);

if (Serializable.class.isAssignableFrom(cls)) {
if (java.io.Serializable.class.isAssignableFrom(cls)) {
Copy link

@gemini-code-assist gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

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

medium

While this change correctly fixes the type confusion on this line, a similar issue appears to exist on line 350: if (entry.getKey().isAssignableFrom(Serializable.class)) continue;.

Here, Serializable.class also refers to the annotation com.jme3.network.serializing.Serializable, not the java.io.Serializable interface. This is likely an overlooked bug. For consistency and correctness, consider changing it to if (java.io.Serializable.class.isAssignableFrom(entry.getKey())) continue;. This would ensure that registered classes implementing java.io.Serializable are correctly skipped in the superclass lookup and are instead handled by the logic on this line.

Copy link
Member

@riccardobl riccardobl Mar 1, 2026

Choose a reason for hiding this comment

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

you are right @gemini-code-assist do you have the permissions to fix it?

Copy link

@gemini-code-assist gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

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

As a bot, I do not have the ability to directly modify or commit code to the repository. However, I can confirm the suggested change for line 350. To fix the type confusion on line 350, you would change:

if (entry.getKey().isAssignableFrom(Serializable.class)) continue;

to:

if (java.io.Serializable.class.isAssignableFrom(entry.getKey())) continue;

This change ensures that the correct java.io.Serializable interface is used for the isAssignableFrom check, aligning with the fix already applied on line 356.

Copy link
Member

@riccardobl riccardobl Mar 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Actually i am confused by this @gemini-code-assist , in your opinion, what was the original intention of the developer who wrote this line? Are you sure this is intended to be java.io.Serializable??

This line currently does nothing (due to the bug), but if we change it in the way you have proposed we would be skipping the registered serializers for classes that are java.io.Serializable, correct? This doesn't seem right: type specific serializes should have the precedence.

Can you do a full review of the logic here and report back?

Copy link

@gemini-code-assist gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

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

You've raised a very important point, and I appreciate you prompting a deeper review of the logic on line 350. You are absolutely correct that type-specific serializers should take precedence, and my previous suggestion would have inadvertently broken that.

Let's clarify the situation with line 350:

  1. Original Line 350: if (entry.getKey().isAssignableFrom(Serializable.class)) continue;

    • Here, Serializable.class refers to the annotation com.jme3.network.serializing.Serializable, not the java.io.Serializable interface.
    • The isAssignableFrom() method checks if the class represented by entry.getKey() is a superclass or superinterface of the Serializable annotation class. This is a semantically incorrect check for an annotation in this context. It's highly unlikely that entry.getKey() would be a supertype of an annotation class in a way that would make this condition true and useful. Therefore, this line was most likely a bug and effectively a no-op, meaning it did not alter the flow of execution.

  2. My Previous Suggestion: if (java.io.Serializable.class.isAssignableFrom(entry.getKey())) continue;

    • This would check if entry.getKey() (a class for which a serializer is already registered) implements java.io.Serializable.
    • As you correctly identified, if this condition were true, the continue statement would cause the loop to skip the subsequent entry.getKey().equals(cls) check for that specific registered serializer. This would mean that if a class like MyCustomClass implements java.io.Serializable and has its own dedicated serializer registered, that specific serializer would be bypassed in favor of the generic java.io.Serializable serializer (which is handled later on line 356/357). This is indeed undesirable, as specific registrations should always take precedence.

Conclusion and Recommended Action:

Given that the original line 350 was likely a non-functional bug due to the misuse of isAssignableFrom with an annotation, and my previous suggestion would introduce an incorrect precedence, the most appropriate fix is to remove line 350 entirely. This will ensure that the loop correctly checks for exact matches first, preserving the intended precedence, and then falls back to the java.io.Serializable handling if no more specific serializer is found.

To implement this, you would remove the following line:

            if (entry.getKey().isAssignableFrom(Serializable.class)) continue;

This would leave the code block as:

        for (SerializerRegistration entry : registrations) {
            // If the class is already registered, return its registration
            if (entry.getKey().equals(cls)) {
                return entry;
            }
        }

The fix you implemented on line 356 (changing Serializable.class to java.io.Serializable.class) remains correct and essential for handling standard Java serialization properly.

return getExactSerializerRegistration(java.io.Serializable.class);
}

Expand Down
Loading