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

Non-ObjectId identifiers generated by event listeners are not populated if documents are inserted as batch [DATAMONGO-1513] #2424

Closed
spring-projects-issues opened this issue Oct 25, 2016 · 4 comments
Assignees
Labels
in: repository type: bug

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Oct 25, 2016

Michael Krog opened DATAMONGO-1513 and commented

We have observed issues when persisting multiple entities at a time. The id does not get populated. It's caused by insertDBObjectList(…) handling ids different compared to insertDBObject(…). In our use case we define our own ids when persisting to Mongo. They are Strings. We use a MongoEventListener to set the id like this:

public void onBeforeSave(BaseEntity p, DBObject dbo) {
        if (p.isNew()) {
            String id = ....;
            dbo.put("_id", id);
        }
    }

This works fine when we persist only one instance at a time because the insertDBObject(…) does not care about the type of the id, but insertDBObjectList(…) does. It goes like this:

if (id instanceof ObjectId) {
    ids.add((ObjectId) id);
} else {
    // no id was generated
    ids.add(null);
}

Result is that we do no get the id populated in the entities we persist


Affects: 1.8.6 (Gosling SR6)

Issue Links:

  • DATAMONGO-1519 Change MongoTemplate.insertDBObjectList(…) to return List<Object> instead of List<ObjectId>

Referenced from: commits e974187, f16809a, 2f522ba, f782338

Backported to: 1.9.5 (Hopper SR5), 1.8.7 (Gosling SR7)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Oliver Drotbohm commented

The thing is that the things don't relate to each other. What both methods — insertDBObjectList and insertDBObject — are supposed to do is return the id(s) generate. The population of domain objects with the identifiers generated is done in populateIdIfNecessary(…). For insertAll(…) on the top level this is eventually done in doInsertBatch(…). For a plain insert(…) it's done in doInsert(…).

I guess the best way forward would be a sample test case that shows what you actually see broken. Care to craft one? Or at least we need a better description of what doesn't work for you

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Oliver Drotbohm commented

I guess I now see where you're heading. The code collecting the ids is only collecting ids that were generated during the insert (at least that was the intention) and fails to return the non-ObjectId identifiers that an event listener might have added. I think we can try to remove that guard and rely on populateIdIfNecessary(…) downstream to not repopulate fields that were already populated (the latter being the thing we actually want to avoid)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Michael Krog commented

That is exactly what I meant.

When a non-ObjectId identifier is set by an event listener the following code will correctly populate the entity with the identifier:

MyEntity e = new MyEntity();
e = repository.save(e);

....but this will NOT populate the entity with the identifier:

List l = new ArrayList();
l.add(new MyEntity());
l = repository.save(l);

...because insertDBObjectList fails to collect non-ObjectID identifiers wheras insertDBObject collects them just fine

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 3, 2016

Oliver Drotbohm commented

The fix is in place already? I guess there's no chance you'll be able to give the snapshots a try in the next couple of hours, right? We're shooting for a Hopper SR today for inclusion in a Boot release tomorrow, so I'd highly appreciate any integration test you could do :)

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

No branches or pull requests

2 participants