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

Caching conflict when creating CSV schemas with different views for the same POJO #288

Closed
mrpiggi opened this issue Sep 17, 2021 · 7 comments
Labels
Milestone

Comments

@mrpiggi
Copy link
Contributor

mrpiggi commented Sep 17, 2021

#195 added csv schema creation with views. Using the same CsvMapper with the same POJO for different views leads to unexpected results as the used view is not considered when a schema is loaded from cache as it was registered for the pojoType. Obviously, the used view should be part of the key for the cache maps _untypedSchemas and _typedSchemas.

protected CsvSchema _schemaFor(JavaType pojoType, LRUMap<JavaType,CsvSchema> schemas,
boolean typed, Class<?> view)
{
synchronized (schemas) {
CsvSchema s = schemas.get(pojoType);
if (s != null) {
return s;
}
}

Edit: A workaround is quite simple. In order to clear the cache, just use csvmapper.copy().schemaForWithView(...) instead of csvmapper.schemaForWithView(...).

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 24, 2021

Oh. Yes indeed... Gnarly one, this. Wonder if this should initially (2.13?) simply avoid caching when there is active View, and then for 2.14 add proper caching with possible view.

@mrpiggi
Copy link
Contributor Author

mrpiggi commented Nov 24, 2021

In my eyes, caching only when view==null would be a sufficient fix for 2.13

@cowtowncoder cowtowncoder changed the title creating csv schemas with different views for the same POJO Caching conflict when creating CSV schemas with different views for the same POJO Dec 15, 2021
@cowtowncoder cowtowncoder added 2.13 and removed 2.14 labels Dec 16, 2021
@cowtowncoder cowtowncoder added this to the 2.13.1 milestone Dec 16, 2021
@cowtowncoder
Copy link
Member

Fixed for upcoming 2.13.1 by simply skipping caching if view enabled. May be improved in future if someone has an itch; just need to change key for cache.

@mrpiggi
Copy link
Contributor Author

mrpiggi commented Dec 16, 2021

What would be suitable to create a corresponding key from JavaType pojoType and Class<?> view for LRUMap? Is there already a generic tuple or something similar to the existing TypeKey integrated in jackson-core or jackson-databind that could be used for this purpose or would a new class (ViewKey?) be necessary?

@cowtowncoder
Copy link
Member

I can't think of a type and since we cannot add 3rd party Pair or Tuple, would need a simple key class like ViewKey.
@mrpiggi if you have time and interest I'd be happy to help merge this to 2.14.

jackson-databind probably has some similar class already fwtw, for (de)serializer caching. Not with exact types but similar.

@mrpiggi
Copy link
Contributor Author

mrpiggi commented Dec 16, 2021

Sure, I can try to find some time. Should I create a PR based on 2.13 or 2.14? Are there any contribution guide lines?

@cowtowncoder
Copy link
Member

2.14: keeping caching simple for 2.13 patch just in case.

There isn't a lot about contributing, just this:

https://github.com/FasterXML/jackson/blob/master/CONTRIBUTING.md

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

No branches or pull requests

2 participants