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

Complete overwrite of existing document - upsert #23

Open
mreis92 opened this issue Dec 4, 2016 · 6 comments
Open

Complete overwrite of existing document - upsert #23

mreis92 opened this issue Dec 4, 2016 · 6 comments

Comments

@mreis92
Copy link

mreis92 commented Dec 4, 2016

Hello! I have noticed in your documentation that we can perform an upsert using a StorageAdapter. My question here is: How to obtain the storage adapter in order to perform this upsert operation?

Thank you!

What you are doing?

Trying to completely replace an existing document.

let storageAdapter = Model.getStorageAdapter();
storageAdapter.upsert(key, value).then((result) => {
  //Do something here
})

What do you expect to happen?

The document to be completely replaced. Using a combination of update or build and replace does not do the trick.

What is actually happening?

I cannot retrieve the storageAdapter

Couchbase SDK version: 2.2.2
Couchbase Server version: 4.1 Community
CouchbaseODM version/branch: v1.0.1

@fogine
Copy link
Owner

fogine commented Dec 4, 2016

Hello @mreis92 ,

According to the documentation, a Model object does not currently have getStorageAdapter method. The method is implemented in the Document prototype which Model's Instance objects inherit. (In other words, the getStorageAdapter is instance method of a Model)

You can retrieve storage adapter of a model from Model.storage property.
Also, note that StorageAdapter.upsert method is just lightweight wrapper of couchbase-sdk.bucket.upsert method. There is no special processing around refDoc indexes for example...

I should probably add brief explanation of individual classes in the doc. I realize now that people who don't know internal implementation design might be confused/unsure what's the purpose of some classes. Thanks.

@mreis92
Copy link
Author

mreis92 commented Dec 4, 2016

Thanks for the prompt reply @fogine .

I understand now, and using Model.storage.upsert did the trick. I also understand that this particular method is a wrapper for the couchbase-sdk method, but I was hoping that I could still leverage some of the ODM capabilities such as key generation and time-stamping (which I see now that it is not the case).

Am I missing some way of achieving what I want while maintaining the aforementioned capabilities? My alternative approach was to do something like:

let saveOptions = {
  isNewRecord: !someId,
  key: someId
}

let model = Model.build(myModel, saveOptions);
return model.save();

,but this did not appear to replace the document as one would expect. In fact, the only time when I can see the changes that I make is when I am creating the document.

Regarding documentation, I feel like a brief description could help, even in exploring the exposed functionalities. Let me know if I can help in the documentation somehow!

@fogine
Copy link
Owner

fogine commented Dec 4, 2016

@mreis92 , you could create your own upsert instance method for now by adding its implementation under instanceMethods option when creating new ODM object.

var couchbase = new couchbaseODM({
    bucket: bucket,
    instanceMethods: {
        upsert: function() {
            var doc = this;
            return this.getGeneratedKey().bind({}).then(function(key) {
                this.key = key;
                return doc.getSerializedData();
            }).then(function(data) {
                return doc.getStorageAdapter().upsert(this.key, data);
            });
        }
    }
});

The above example adds upsert method to your model variable (every Instance object).

I have been thinking about adding support for upsert method however I struggle with some implementation design details. Particularly with refDoc indexes and how they should be treated when performing upsert operation.
Let's say you have User model which has refDoc index defined on username property. Suppose you have one document in a bucket with username=fogine. Now, when you call the upsert method on a brand new Instance object which has also username property set to fogine (upsert will internally perform insert in this particular case - creating new document in the bucket), there is a conflict with the reference document because another document with the same username value already exists. They are few options how to handle this: overwrite also all conflicting reference documents so that they reference to new document which has been created by the upsert, another option would be to ignore refDoc indexes and in case of conflict, return an Error. There might be a method option for that specifying which behavior to take. However this would increase complexity quite a lot, considering there is application logic which handles rollback of failed operations which have mutated two or more documents.

About the documentation, if you think there is something which could be better/more documented or a specific tutorial is missing which would have helped you with understanding the ODM, I'll happily accept a PR if you feel like posting one :)

@fogine
Copy link
Owner

fogine commented Dec 4, 2016

but this did not appear to replace the document as one would expect. In fact, the only time when I can see the changes that I make is when I am creating the document.

@mreis92 , the code example you provided should preform replace operation and replace document's data by those define in myModel. Check that isNewRecord=false and that your key option has correct id value set otherwise it's gonna try create new document. In case, you're sure that your values are correct and you suspect possible bug, you did not describe what is actually happening, SSCCE would be very descriptive.

@fogine
Copy link
Owner

fogine commented Dec 5, 2016

@mreis92 , I've just realized that my response above stating that building new model instance with isNewRecord and then calling save on it should perform replace is not fully correct, my apologies. In the v1 it also needs cas value to be set (see getCAS / setCAS methods of the ODM Document) which in your case might be a problem if your don't load your data from a bucket.
This issue exists due to incomplete model associations feature implementation and is addressed in upcoming v2 release.

@mreis92
Copy link
Author

mreis92 commented Dec 6, 2016

@fogine It makes sense. For my particular use case, I wound't have that problem because I don't have refDocs for that particular model.

IMO, overwriting existing refDocs might lead to inconsistencies or unexpected behaviour (depends on the scenario). A less "performant" approach would be to check existing refDocs on upsert, and abort if one of the refDocs already exists - I agree that this would also increase the complexity of the approach.

I've noticed in the documentation that the cas value is optional for the replace, so I was assuming that it would get the first document it would find, with a valid cas. I might take the upsert approach for now, since I don't have the issue with refDocs for my particular use case.

PS: Regarding documentation, I'll see if I can contribute on some meaningful examples (if not in the method documentation, maybe add some more examples to the tutorials :) )

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

No branches or pull requests

2 participants