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

GENERIC_TYPE_NOT_PARAMETRIZED #76

Closed
ctmay4 opened this Issue Jan 6, 2015 · 23 comments

Comments

Projects
None yet
2 participants
@ctmay4

ctmay4 commented Jan 6, 2015

This is probably a lack of understanding on my part, but I am getting this error trying to commit changes to the repository:

JaversException: GENERIC_TYPE_NOT_PARAMETRIZED JaVers runtime error - expected actual Class arguments in type 'java.util.List<java.util.List<java.lang.String>>'. Javers needs to know what kind of content is stored in your collections. Try at least <Object>

It doesn't seem to like the fact that I have a field that is a List<List>. The field in question looks like this (that is a Morphia annotation):

@Property("rows")
private List<List<String>> _rows;

and the getter/setter look like this (that is a Jackson annotation):

@JsonProperty("rows")
public List<List<String>> getRawRows() {
    return _rows;
}

public void setRawRows(List<List<String>> rows) {
    _rows = rows;
}

Do I have to register that property when I create my javers instance? Here is how I build it:

_javers = JaversBuilder.javers()
        .registerValueGsonTypeAdapter(ObjectId.class, new ObjectIdAdapter())
        .withMappingStyle(MappingStyle.BEAN)
        .registerEntity(StagingSchema.class, "internalId")
        .registerEntity(StagingTable.class, "internalId")
        .registerJaversRepository(new MongoRepository(getDatastore().getDB()))
        .build();

I looked in the documentation but only saw property configuration for ignoring a field or setting a field to be an identifier.

@bartoszwalacik bartoszwalacik self-assigned this Jan 7, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 7, 2015

yeah, looks like that error message is not very informative in this case. I'll change this message.

But te be honest, List<List<String>> is not what we can support for now.
Not sure how we could model a Change for this type.

Are you able to change this data model, and introduce a new ValueObject class?
Or is it required by some framework you use?

For example to sth like this

private List rows_;

public class StringList {
private List strings;
}

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 7, 2015

I might be able to change it, but it is going to be alot of work. There are alot of things that already use it. Why is it easier to support a List where sub-class only has a List vs just having a List<List>?

Is there a way that I could say to ignore it so I could continue my testing?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 7, 2015

as a quick workaround you can add @DiffIgnore to the field. Give me a while to think out how to solve this problem better

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 20, 2015

Any more thoughts on this? It is the last show stopper for me. The issue is that I want my raw data to look like this:

{
  "rows": [
    ["A", "1"],
    ["B", "2"],
    ["C", "3"]
   ]
}

Since I use Jackson, the entity is annotated like this:

    @Property("rows")
    private List<List<String>> _rows;

I could change the entity to look like this, which I think would work with your library:

    @Property("rows")
    private List<TableRow> _rows;

and define TableRow as an entity with a single property, which is a List. However then the output would look like this:

{
  "rows": [
    { "row": ["A", "1"] },
    { "row": ["B", "2"] },
    { "row": ["C", "3"] }
   ]
}

which is not my preference.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 21, 2015

Hi Chuck,
in the new version we have Custom Comparators.
see http://javers.org/javadoc_1.0.0/org/javers/core/JaversBuilder.html#registerCustomComparator-org.javers.core.diff.custom.CustomPropertyComparator-java.lang.Class-
Also we have Custom Json Serializers.
see http://javers.org/documentation/repository-examples/#json-type-adapter

you can use them for a TableRow class

You mean output from Diff serialization or Snapshot from Mongo?

Maybe you can come up with some working example on github, which would describe your case.
It would be easier for me to help you and decide if this only the matter of configuration or does we need a new feature

regards

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 22, 2015

I'm not sure how those would work, but it sounds like they would all involve me changing my entity from looking like this:

    @Property("rows")
    private List<List<String>> _rows;

to this:

    @Property("rows")
    private List<TableRow> _rows;

Which would also change the source JSON of my entities into a format I do not like.

If there is no way to support registering a class to handle the diffs between a List<List> then I may be stuck.

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 22, 2015

I did some more testing and put together a more simple complete example:

https://gist.github.com/ctmay4/df042868def85a380ff8

In that example, I also tested the same object using another library as a point of reference (https://github.com/SQiShER/java-object-diff).

Ideally, there would be a way in JaVers to register how to deal with a List in my example.

Let me know if you need anything else from me.

bartoszwalacik added a commit that referenced this issue Jan 22, 2015

#76
failing test added
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 23, 2015

your gist is fine, I'm starting to work on this issue

bartoszwalacik added a commit that referenced this issue Jan 28, 2015

#76
changing ItemType of Collections and Maps from raw Class to Type (in order to support Generics  as ItemType)

bartoszwalacik added a commit that referenced this issue Jan 28, 2015

#76
fixing tests

bartoszwalacik added a commit that referenced this issue Jan 28, 2015

#76
fixing tests

bartoszwalacik added a commit that referenced this issue Jan 28, 2015

#76
tests fixed
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 29, 2015

Hi Chuck,
issue is done and available as snapshot version 1.0.5-SNAPSHOT
in sonatype repo: https://oss.sonatype.org/content/repositories/snapshots/

check it and if it works fine, we release it

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 29, 2015

I retested my gist and it worked great.

commit 2.0, author: me@here.com, Jan 29, 2015 1:36:47 PM
  changed object: com.imsweb.seerapi.DiffTesting$TestObject/1
    value changed on '_name' property: 'A Name' -> 'A Different Name'
    list changed on '_tags' property: [(0).added:'tag1', (1).added:'tag2']
    list changed on '_table' property: [(1).'[D, E, F]'>>'[X, Y, Z]']
commit 1.0, author: me@here.com, Jan 29, 2015 1:36:46 PM
    new object: com.imsweb.seerapi.DiffTesting$TestObject/1

I will do some more testing this afternoon. Thanks!

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 29, 2015

Does there need to be a Mongo repository snapshot to go along with the core changes? I tried to to a more "real" test and I am getting these errors. I'm pretty sure I was not getting them before.

java.lang.NullPointerException
    at org.javers.repository.mongo.MongoRepository.writeToDBObject(MongoRepository.java:166)
    at org.javers.repository.mongo.MongoRepository.persistSnapshots(MongoRepository.java:62)
    at org.javers.repository.mongo.MongoRepository.persist(MongoRepository.java:54)
    at org.javers.repository.api.JaversExtendedRepository.persist(JaversExtendedRepository.java:54)
    at org.javers.core.Javers.commit(Javers.java:79)
    at com.imsweb.seerapi.ejb.StagingEJB.saveTable(StagingEJB.java:643)
    at com.imsweb.seerapi.ejb.StagingEJBTest.addData(StagingEJBTest.java:163)
    at com.imsweb.seerapi.ejb.StagingEJBTest.testGetAllTablesWithQuery(StagingEJBTest.java:344)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 30, 2015

right, NPE is fixed,
please check fresh snapshot version 1.0.5-SNAPSHOT

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 30, 2015

That fixed the NPE. Thanks.

I noticed that my units tests were running much slower. I boiled it down to a singe table. When I save this table, I get the following in the log:

[main] INFO org.javers.core.Javers - Commit(id:1.0, snapshots:1686, changes - NewObject:1686)

Just saving a single table is putting 1686 entries into the snapshot table. Is that normal? The operation is pretty slow (over 5 seconds for this table using a local instance of MongoDB). Here is the the table entity so you know what it looks like. I cut out many of the rows for brevity, but there are 840 rows in the original object. It kind of seems there are two snapshots for every row.

{
    "id": "testis_ajcctnm7_stage",
    "title": "AJCC TNM 7 Stage",
    "definition": [
        { "key": "t", "name": "T", "type": "INPUT" },
        { "key": "n", "name": "N", "type": "INPUT" },
        { "key": "m", "name": "M", "type": "INPUT" },
        { "key": "s", "name": "S", "type": "INPUT" },
        { "key": "stage", "name": "Stage", "type": "ENDPOINT" }
    ],
    "rows": [
        [ "T0", "N0", "M0", "S0", "VALUE:ERROR" ],
        [ "T0", "N0", "M0", "S1", "VALUE:IS" ],
        [ "T0", "N0", "M0", "S2", "VALUE:IS" ],

<SNIP>

        [ "TX", "NX", "M1NOS", "S3", "VALUE:IIINOS" ],
        [ "TX", "NX", "M1NOS", "SX", "VALUE:IIINOS" ],
        [ "TX", "NX", "M1NOS", "ERROR", "VALUE:ERROR" ]
    ],
    "last_modified": "2014-03-11T18:57:19.519Z"
}

Considering I have 1000's or tables, the size of the snapshot collection would grow extremely large very quickly. Is this how you expected this to be persisted?

Thanks again for all your help.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 31, 2015

ok, so you are comparing your app performacne with and without javers?
the key information is
Commit(id:1.0, snapshots:1686, changes - NewObject:1686)
It means, Javers detects 1686 distinct objects in the object graph you are persisting.

Is your graphs really such large or maybe there are some noise data like dynamic proxies or auto-generated code?

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 31, 2015

The object is the one above except with 840 rows in the definition. I think the reason it is basically doubled is because I have an internal representation of the "definition" that is constructed for searching.

So 2 questions:

  1. Is there a way to tell JaVers to ignore certain properties for comparison? I could probably null out the fields before committing them, but I didn't know if there was a more elegant way.
  2. I guess I didn't realize that each object in the entity was persisted in the history. I was thinking in my head we were only talking about diffs, but I didn't think about the fact that you are saving all objects for the initial commit. When I've implemented similar functionality in other systems, I just recorded the first commit without copying the object. Then I tracked diffs on each subsequent commit.

I have concerns about size, but honestly the speed hit is a bigger concern for my use case. Once I figure out a way to not track diffs for the internal duplicated fields (which I think will make a big difference for me in both size and speed), I will need to figure what the real speed impact is.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 31, 2015

Answering your questions.

  1. Yes you can easily ignore certain properties using annotations (@transient, @DiffIgnore). Check javers.org/documentation/configuration for details.
  2. Javers has to persist snapshots of all objects given to initial commit. This is the only way to calculate the diff during second commit. But after second commit javers
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 31, 2015

... After second commit , javers persists only snapshots of changed objects. Snapshots for unchanged objects are reused. This saves lot of db space.

If performance is critical in you case , try to split your big object graph to smaller chunks. Each object in graph means one insert or select

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 31, 2015

Soryy , writing on phone.

is this answer helpful?

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 31, 2015

On a phone too.

Yes I think I understand. As I said there are some large objects on the entity that I would like to exclude from JaVers. Can I tell it during configuration to ignore them? Once those parts are not included I will be able to better evaluate.

You're comments have been very helpful. I think the original issue can definitively be closed. Thanks again.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 31, 2015

For now you can ignore objects using field/getter annotations @DiffIgnore or @transient (refer to documentation for details). Are you thinking about another way of configuring ignored objects?

I'll close this issue after release 1.0.5 to maven central

@ctmay4

This comment has been minimized.

ctmay4 commented Jan 31, 2015

Sometimes you don't have access to the objects to add annotations. It would be a big improvement if you could programmatically ignore properties.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 1, 2015

Sounds like a good idea, please submit new issue for this feature

bartoszwalacik added a commit that referenced this issue Feb 1, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 1, 2015

v 1.0.5 released to maven central

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment