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

Support for @JsonValue custom ID types #224

Closed
marquiswang opened this issue Jul 12, 2022 · 12 comments
Closed

Support for @JsonValue custom ID types #224

marquiswang opened this issue Jul 12, 2022 · 12 comments
Milestone

Comments

@marquiswang
Copy link

I'm finally trying to migrate from 2.x. I have a lot of code of the form:

JacksonDBCollection.wrap(collection, SomeType.class, CustomId.class, objectMapper);

where CustomId is a class with @JsonValue and @JsonCreator annotations that allow Jackson to serialize it to a string. With 2.x, this was supported and used the String serialization of CustomId as the id.

With 3.x and 4.x, the generic key type has been removed from JacksonMongoCollection, and I get the following error when trying to insert:

java.lang.IllegalArgumentException: Unsupported ID type: class foo.bar.CustomId

Is there any way to get the equivalent old functionality?

@rlodge
Copy link
Contributor

rlodge commented Jul 12, 2022

Not at this time, at least not without fakery inside the class (like declaring the _id field as String and then converting to/from it...)

@marquiswang
Copy link
Author

Thanks, that's what I was afraid of. Is this something that could be added?

@rlodge
Copy link
Contributor

rlodge commented Jul 12, 2022

I think so. It involves determining if the jackson-converted value will be something mongo will use as an id, though, so the solution will have to be thought through a bit. I played with some options this morning but I don't know if I like them.

@marquiswang
Copy link
Author

I managed to make it work by adding

try {
    final String stringRepresentation = objectMapper.writer().writeValueAsString(value);
    if (stringRepresentation.startsWith("\"") && stringRepresentation.endsWith("\"")) {
        return new BsonString(stringRepresentation.substring(1, stringRepresentation.length() - 1));
    }
} catch (JsonProcessingException aE) {
    throw new RuntimeException(aE);
}

at the bottom of JacksonCodec#constructIdValue. I don't know if there is a more elegant way to determine if Jackson thinks an object should be serialized as a non-object type (I'm only interested in Strings here, but obviously we could detect numeric types too).

@rlodge
Copy link
Contributor

rlodge commented Jul 12, 2022

I did something a bit similar:

           try {
                final JsonNode tree = objectMapper.valueToTree(value);
                if (tree.isTextual()) {
                    return new BsonString(tree.asText());
                }
            } catch (RuntimeException e) {
                throw new IllegalArgumentException(String.format("Unsupported ID type: %s", value.getClass()), e);
            }

And I wrote some tests.

But I wanted to support the other types and I ran into test problems that seem related to newer JDKs, and then I had to move onto assigned work. :) I'll see if I can get to it more seriously soon.

@marquiswang
Copy link
Author

I'm afraid to report that I've found further code in my codebase (yay legacy codebases) that uses an id field with an object, which apparently mongojack 2.x and Mongo happily allow (totally didn't know that).

> db.foo.bar.findOne()
{
	"_id" : {
		"foo" : "bar",
		"date" : [
			2016,
			4,
			5
		]
	},
	"offset" : 123.456
}

If we want MongoJack to support this... that would be really nice (and backwards compatible with 2.x!). Though I could understand if you didn't want to.

(BTW, I found that any version of Jackson below 2.13.3 does not work with Java 17, in case that's what you're running into)

@marquiswang
Copy link
Author

I found that this works

        final JsonNode tree = objectMapper.valueToTree(value);
        if (tree.isDouble()) {
            return new BsonDouble(tree.asDouble());
        } else if (tree.isTextual()) {
            return new BsonString(tree.asText());
        } else if (tree.isInt()) {
            return new BsonInt32(tree.asInt());
        } else if (tree.isLong()) {
            return new BsonInt64(tree.asLong());
        } else if (tree.isBigDecimal()) {
            return new BsonDecimal128(new Decimal128(tree.decimalValue()));
        } else {
            return BsonDocument.parse(tree.toString());
        }

@rlodge
Copy link
Contributor

rlodge commented Jul 15, 2022

I have published a 4.5.1-SNAPSHOT version that I believe fixes this. Please let me know if it works for you.

@marquiswang
Copy link
Author

Thanks so much! Trying this out now.

@marquiswang
Copy link
Author

This does appear to work for me! Thanks!

@rlodge rlodge added this to the 4.5.1 milestone Jul 22, 2022
@rlodge
Copy link
Contributor

rlodge commented Jul 22, 2022

Fix deployed 4.5.1 and 4.7.0

@rlodge rlodge closed this as completed Jul 22, 2022
@marquiswang
Copy link
Author

Thank you!

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

2 participants