`SimpleModule` not registered due to `getTypeId()` returning an empty string

[seadbrane]

Search before asking

• I searched in the issues and found nothing similar.

Describe the bug

I think the changes made for #3110 have resulted in some unintended behavior.

Specifically, the new SimpleModule(Version) constructor calls this(version.getArtifactId(), version); which sets _hasExplicitName = true even though no explicit name was passed in. And since getArtifactId() returns "" instead of null, this results in getTypeId() returning the same "" for distinct modules.

I think either _hasExplicitName name should not be set to true when it is not explicitly passed in, or _name should be set to null if getArtifactId() returns empty string. The former is probably easier since there validation in ObjectMapper to ensure getModuleName() is not null, although it doesn't really seem correct to implicitly set name to an empty string - especially when it can also result in getTypeId() being an empty string.

Here is a simple test case:

  public class Module1 extends SimpleModule {
        public Module1() {
            super(Version.unknownVersion());
        }
    };
    
    public class Module2 extends SimpleModule {
        public Module2() {
            super(Version.unknownVersion());
        }
    };
    
    @Test
    public void test() {
        ObjectMapper mapper = new ObjectMapper();
        mapper.registerModule(new Module1());
        mapper.registerModule(new Module2());
        Set<Object> modules = mapper.getRegisteredModuleIds();
        Assert.assertEquals(2, modules.size());
    }

Version Information

No response

Reproduction

<-- Any of the following

1. Brief code sample/snippet: include here in preformatted/code section
2. Longer example stored somewhere else (diff repo, snippet), add a link
3. Textual explanation: include here
   -->

// Your code here

Expected behavior

No response

Additional context

No response

[seadbrane]

We ran into another related issue due to using the same name in multiple modules. In this case, it looks the name was copy/pasted - so it is an easy fix to use a different name to get around it. At the same time, using name for the typeId seems a bit problematic, and the change from defaulting typeId from the module class, which was guaranteed to not conflict, to name has increased the probability of an issue. Since this could result in unexpected (bad data) serialized output, I really think this should fail fast rather than silently not register a module.

Here is another example test that fails.

  public class MyModule1 extends SimpleModule {
        public MyModule1() {
            super("MyModule", new Version(1, 0, 0, null, null, null));
        }
    };
    
    public class MyModule2 extends SimpleModule {
        public MyModule2() {
            super("MyModule", new Version(2, 0, 0, null, null, null));
        }
    };
    
    @Test
    public void test2() {
        ObjectMapper mapper = new ObjectMapper();
        mapper.registerModule(new MyModule1());
        mapper.registerModule(new MyModule2());
        Set<Object> modules = mapper.getRegisteredModuleIds();
        Assert.assertEquals(2, modules.size());
    }

[cowtowncoder]

Ok: I think the first part -- not marking name as explicit when it is not -- makes sense.

Would it make sense to change things so that if:

1. When no name is passed AND version.getArtifactId() returns null or "", id is auto-generated same as in no-args-constructor case ("SimpleModule-"+MODULE_ID_SEQ.getAndIncrement())
2. If name passed explicitly with null or ""... we'll either
   • Throw IllegalArgumentException OR
   • Create default name?

On using same name, that's... fundamentally difficult to change. But there is MapperFeature.IGNORE_DUPLICATE_MODULE_REGISTRATIONS that can be disabled to allow all registrations.

I think I'll consider this issue to be just for the originally reported problem, hence doing (1) from above.
For other work let's create follow-up issues.

[cowtowncoder]

@seadbrane As per my comment, fixed the initial reported problem. I am not sure what to think about the second part -- I can see the concern, but at the same time, name as passed is assumed as unique type id and is by now the way things work so changing it might cause problems for existing use cases.

But we could consider changes via another issue at any rate -- perhaps uniqueness check should start by checking implementation class and secondly given name. I think (could be wrong) that the change to use name was for sort of reverse purpose: ability for instances of same class to be distinguished as different modules. But the intent was not necessarily to make instances of different classes to be taken as "same" module.

And in the end the whole unique/same checking is to try to help in accidental re-registrations, to guard against user error.

[seadbrane]

Thanks for the fix.

On the second part, there is really nothing that indicates that getModuleName() is or should be associated with getTypeId(). In fact, it is mostly the opposite according to the javadocs. I understand fully rolling that back now my be difficult, at the same time, I also think failing fast as part of an upgrade would be preferable to silently not registering.

Related - not entirely sure what this is trying to say, although my interpretation is that it would probably return null or the class name.

Since instances are likely to be custom, implementation returns null if (but only if!) this class is directly instantiated; but class name (default impl) for sub-classes.

[cowtowncoder]

Ok, open to Javadoc improvements as well. Will try to do first pass now.

[cowtowncoder]

Oh boy. Yes, Javadocs and comments are seriously misaligned wrt actual code.

Also looks like use of null, while documented, is not good wrt ObjectMapper.getRegisteredModuleIds() as such modules ids are not added.

All of this said I think I'd like to keep behavior itself about as it is now for 2.x and correct Javadocs.

For 3.0 more changes are possible.