Skip to content
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

Error when trying to deserialize BSON into a EXISTING_PROPERTY polymorphic class #67

Closed
jonpeterson opened this issue Sep 1, 2016 · 16 comments

Comments

@jonpeterson
Copy link

I ran into an problem when trying to deserialize BSON into a polymorphic class that uses JsonTypeInfo.As.EXISTING_PROPERTY.

Groovy script:

@Grab('de.undercouch:bson4jackson:2.7.0')

import com.fasterxml.jackson.annotation.JsonSubTypes
import com.fasterxml.jackson.annotation.JsonTypeInfo
import com.fasterxml.jackson.databind.ObjectMapper
import de.undercouch.bson4jackson.BsonFactory
import de.undercouch.bson4jackson.BsonModule

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = 'type')
@JsonSubTypes([
    @JsonSubTypes.Type(name = 'a', value = A),
    @JsonSubTypes.Type(name = 'b', value = B)
])
abstract class Base {
    UUID id
    String type
}

class A extends Base {}

class B extends Base {}


def mapper = new ObjectMapper(new BsonFactory()).registerModule(new BsonModule())
def bytes = mapper.writeValueAsBytes(new A(id: UUID.randomUUID()))
mapper.readValue(bytes, Base)

Throws the stacktrace:

Caught: com.fasterxml.jackson.core.JsonGenerationException: BsonSerializer can only be used with BsonGenerator
com.fasterxml.jackson.core.JsonGenerationException: BsonSerializer can only be used with BsonGenerator
        at de.undercouch.bson4jackson.serializers.BsonSerializer.serialize(BsonSerializer.java:37)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:130)
        at com.fasterxml.jackson.databind.ObjectMapper.writeValue(ObjectMapper.java:2436)
        at com.fasterxml.jackson.databind.util.TokenBuffer.writeObject(TokenBuffer.java:849)
        at com.fasterxml.jackson.databind.util.TokenBuffer.copyCurrentEvent(TokenBuffer.java:1006)
        at com.fasterxml.jackson.databind.util.TokenBuffer.copyCurrentStructure(TokenBuffer.java:1048)
        at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:97)
        at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserializeWithType(AbstractDeserializer.java:142)
        at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:42)
        at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3788)
        at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2872)
        at com.fasterxml.jackson.databind.ObjectMapper$readValue$1.call(Unknown Source)
        at so.run(so.groovy:27)

Hints:

  • When I use JsonTypeInfo.As.PROPERTY the issue goes away.
  • When I don't register BsonModule the issue goes away.
  • This was occurring on JDK 7 but not JDK 8. After trying to debug a bit, this appears to be because of reflection field order.
@michel-kraemer
Copy link
Owner

This is interesting. I'm not sure if this is an issue with bson4jackson or Jackson maybe. @cowtowncoder What do you think?

@jonpeterson
Copy link
Author

  • When I don't register BsonModule the issue goes away.

@michel-kraemer
Copy link
Owner

If you don't register BsonModule then you can't serialize the UUID field.

After trying to debug a bit, this appears to be because of reflection field order.

Can you elaborate?

@cowtowncoder
Copy link

Looking at the stack trace:

Caught: com.fasterxml.jackson.core.JsonGenerationException: BsonSerializer can only be used with BsonGenerator
com.fasterxml.jackson.core.JsonGenerationException: BsonSerializer can only be used with BsonGenerator
        at de.undercouch.bson4jackson.serializers.BsonSerializer.serialize(BsonSerializer.java:37)

this is due to BsonSerializer not wanting to write to a TokenBuffers JsonGenerator implementation, which is needed for buffering. Buffering is needed in cases where type id value comes after contents of the object for which type id is needed.

Most likely BsonSerializer will need to support this case; TokenBuffer itself knows nothing about extensions (and can not).

@jonpeterson
Copy link
Author

jonpeterson commented Sep 1, 2016

IRT my comment about reflection field ordering, I was just trying to provide a hint to help narrow down the potential bug. I was able to reproduce the error on JDK 8, but there was a case where I was only seeing the error happen on JDK 7. After renaming the type field it errored on both. When stepping through the debugger, it appears to be occurring here. If the token for type was evaluated before the UUID everything was good; otherwise the TokenBuffer was created and passed to BsonSerializer.

@michel-kraemer
Copy link
Owner

@cowtowncoder I see. Thanks for the detailed explanation!

What do you suggest? Letting BsonSerializer write to a JsonGenerator doesn't make much sense since we want to write BSON and not JSON. I suppose if BsonGenerator would create it's own instance of BsonGenerator then this would eliminate the buffering, which is necessary to support the whole use case, if I understand you correctly.

@cowtowncoder
Copy link

@michel-kraemer In this case it all comes from the need to copy tokens from BSON-backed parser, write in TokenBuffer (using its JsonGenerator implementation), and then, once needed, reading it back. So content is not really being encoded, but the only way to buffer in generic fashion is to use JsonParser/JsonGenerator interfaces.
Further I am guessing that there's a BSON-specific value involved; otherwise writeObject() would not need to be called. It's this particular call that is problem.
I don't think any addition generator types would help; there is no real encoding happening.

Anyway, so copyCurrentStructure() (and ..event()) is the method called, and it encounters writeEmbeddedObject(), which must be copied (before 2.8 at least... more below) by calling writeValue(), and this gets things re-routed to cause the problem.

Now: on solution: one possibility would be to make BsonSerializer consider special case where generator given is TokenBuffer, and let that pass. However... given that the value is apparently BSON-specific, I am not 100% sure that is possible.

Which brings me to one relevant 2.8 addition: method writeEmbeddedObject(), added in JsonGenerator. I think that should be used by either TokenBuffer or by BsonSerializer... or maybe both. BSON generator also needs to support it, so that it is possible to write native values (I think there are couple of such opaque types?).
I'll see what I can do with TokenBuffer; there are some complexities as default generator actually has no direct support for embedded objects as of 2.8.2. But it should be possible to at least make it aware of binary data, similar to how writeObject() works.

@cowtowncoder
Copy link

Ok: implemented these

FasterXML/jackson-databind#1361
FasterXML/jackson-core#318

so that writeEmbeddedObject() should be passed through and this may either solve the problem, or at least make it easier to solve if there are remaining issues. Fixes will be in 2.8.3 as 2.7 did not have matching functionality to use.

michel-kraemer added a commit that referenced this issue Sep 4, 2016
@michel-kraemer
Copy link
Owner

michel-kraemer commented Sep 4, 2016

@cowtowncoder Thanks Tatu, for the help and for fixing this so quickly. I tested the code from @jonpeterson with the current Jackson snapshot, but the problem persists.

I then wanted to debug bson4jackson and tried to write a unit test to reproduce the error, but everything worked correctly. I wasn't able to reproduce the error in a unit test written in Java.

@jonpeterson Here's my unit test:
https://github.com/michel-kraemer/bson4jackson/blob/issue67/src/test/java/de/undercouch/bson4jackson/Issue67Test.java

Can you please check if this code does what you intent? If yes, I suppose it must have something to do with the way Groovy generates classes. I used javap to get the properties Groovy generates for Base. In addition to the UUID and the type it also creates properties called property and metaClass.

$ javap Base
Compiled from "test.groovy"
public abstract class Base implements groovy.lang.GroovyObject {
  public static transient boolean __$stMC;
  public Base();
  static {};
  protected groovy.lang.MetaClass $getStaticMetaClass();
  public groovy.lang.MetaClass getMetaClass();
  public void setMetaClass(groovy.lang.MetaClass);
  public java.lang.Object invokeMethod(java.lang.String, java.lang.Object);
  public java.lang.Object getProperty(java.lang.String);
  public void setProperty(java.lang.String, java.lang.Object);
  public java.util.UUID getId();
  public void setId(java.util.UUID);
  public java.lang.String getType();
  public void setType(java.lang.String);
}

@jonpeterson
Copy link
Author

Add @JsonPropertyOrder({"uuid", "type"}) to TypeAsPropertyWithUUIDBase to force the issue to occur. It's a pseudo-non-deterministic issue based on OS, JVM version, etc. That's what I was talking about when I said in my original post that it happened to be occurring on JDK 7 and not 8.

@michel-kraemer
Copy link
Owner

OK. I'm now able to reproduce it in my unit test. Thanks for the hint.

I think I'm able to fix it quite easily. I'll try to make a commit tonight.

@bguerout
Copy link

bguerout commented Sep 15, 2016

Hello,

I think we have encountered the same bug in Jongo : bguerout/jongo#257 and bguerout/jongo#288

@michel-kraemer
Copy link
Owner

Thanks, @bguerout! This was exactly what I was looking for. I implemented the solution for this issue in a similar same way.

@jonpeterson the issue should be solved in master. It will only work correctly with Jackson 2.7.0 or higher

@michel-kraemer
Copy link
Owner

I just uploaded a new snapshot including the fix:
https://oss.sonatype.org/content/repositories/snapshots/de/undercouch/bson4jackson/2.8.0-SNAPSHOT/

@cowtowncoder
Copy link

@michel-kraemer Would it be possible to release official 2.8.0, with Jackson 2.8(.6?) dependency?
2.8 has been out for a while.

@michel-kraemer
Copy link
Owner

I just release bson4jackson 2.8.0. Sorry that it took me so long. Please note that version 2.9.0 will also be released later today or tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants