search

lightblue-platform / lightblue-mongo

MongoDB backend for lightblue
GNU General Public License v3.0
3 stars 14 forks source link

Fix #359: merge adds references of elements from the old doc into the… #360

Closed bserdar closed 7 years ago

bserdar commented 7 years ago

… new doc, and one of these references is the version info, so any modifications to the version info after the merge modifies both docs

bserdar commented 7 years ago

Actually, it is not preferable. A deep copy is preferable. Let me think about this a bit.

On Mon, May 15, 2017 at 10:08 AM, Dennis Crissman notifications@github.com wrote:

@dcrissman commented on this pull request.

In mongo/src/main/java/com/redhat/lightblue/mongo/crud/ MongoSafeUpdateProtocol.java https://github.com/lightblue-platform/lightblue-mongo/pull/360#discussion_r116533136 :

@@ -272,7 +272,15 @@ public void retryConcurrentUpdateErrorsIfNeeded(Map<Integer,Error> results) { if(updatedDoc!=null) { // if updatedDoc is null, doc is lost. Error remains DBObject newDoc=reapplyChanges(index,updatedDoc);

  • // WARNING There is a potential for major screwup
  • // here, so be careful when you're changing this
  • // code. reapplyChanges implementation calls merge(),
  • // which adds a reference to @mongoHidden at the root
  • // level in the old document into the new
  • // document. This is NOT a copy, a reference. So, the
  • // setDocVer call below sets the version of BOTH docs

It might be worth stating why a reference is preferable to a copy.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lightblue-platform/lightblue-mongo/pull/360#pullrequestreview-38161592, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgDDZ-30IAWrpH3eUMlCp7Kd9HjGDHYks5r6HhngaJpZM4NZ7Rq .

bserdar commented 7 years ago

This is now updated.

On Mon, May 15, 2017 at 10:12 AM, Burak Serdar bserdar@ieee.org wrote:

Actually, it is not preferable. A deep copy is preferable. Let me think about this a bit.

On Mon, May 15, 2017 at 10:08 AM, Dennis Crissman < notifications@github.com> wrote:

@dcrissman commented on this pull request.

In mongo/src/main/java/com/redhat/lightblue/mongo/crud/MongoSaf eUpdateProtocol.java https://github.com/lightblue-platform/lightblue-mongo/pull/360#discussion_r116533136 :

@@ -272,7 +272,15 @@ public void retryConcurrentUpdateErrorsIfNeeded(Map<Integer,Error> results) { if(updatedDoc!=null) { // if updatedDoc is null, doc is lost. Error remains DBObject newDoc=reapplyChanges(index,updatedDoc);

  • // WARNING There is a potential for major screwup
  • // here, so be careful when you're changing this
  • // code. reapplyChanges implementation calls merge(),
  • // which adds a reference to @mongoHidden at the root
  • // level in the old document into the new
  • // document. This is NOT a copy, a reference. So, the
  • // setDocVer call below sets the version of BOTH docs

It might be worth stating why a reference is preferable to a copy.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lightblue-platform/lightblue-mongo/pull/360#pullrequestreview-38161592, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgDDZ-30IAWrpH3eUMlCp7Kd9HjGDHYks5r6HhngaJpZM4NZ7Rq .

dcrissman commented 7 years ago

@bserdar - I don't see that any new changes were pushed up. Am I missing something?

bserdar commented 7 years ago

No, I pushed it to wrong branch. It is there now.

On Tue, May 16, 2017 at 12:06 PM, Dennis Crissman notifications@github.com wrote:

@bserdar https://github.com/bserdar - I don't see that any new changes were pushed up. Am I missing something?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lightblue-platform/lightblue-mongo/pull/360#issuecomment-301866021, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgDDdVMKxqqz45Bb-7yREIY0zOlFZJ4ks5r6eWKgaJpZM4NZ7Rq .

dcrissman commented 7 years ago

I am good with this change now. I just had one question and there seems to be a merge conflict.

bserdar commented 7 years ago

both fixed.

On Tue, May 16, 2017 at 12:21 PM, Dennis Crissman notifications@github.com wrote:

I am good with this change now. I just had one question and there seems to be a merge conflict.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lightblue-platform/lightblue-mongo/pull/360#issuecomment-301871001, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgDDaAKYKN_3MbPvLP_uoBGM2RBi4qRks5r6ekzgaJpZM4NZ7Rq .