Add support to use PUT rest api

[muyadan]

No description provided.

[rs]

PUT is already implemented in REST layer and does not need any extra method at storer level (see https://github.com/rs/rest-layer/blob/master/rest/method_item_put.go#L89).

[muyadan]

The PUT there is not using put method but use post instead. I just created a pull request to use Replace (which use PUT).

[rs]

Sorry I don't get you point. The current PUT is inserting an item if it does not exist already or replace it if it does exist, making sure we respect revision restrictions. I'm not sure to understand what you are trying to do.

[muyadan]

In order to use elastic search, it does not support replacing item using POST method which is the current implementation when you use Update from storer elastic search api. https://github.com/olivere/elastic/blob/release-branch.v5/update.go

Which is not using PUT but use Post and resulting in partial updating the fields.

I created a pull request to olivere elastic to use PUT.
olivere/elastic#501

[rs]

Ok I think I get it. REST layer is doing the "partial update" on its side when necessary, so it does not expect the storer to partially update the document. The problem you are describing should thus only happen within the ES storer, when you remove a field from a document. Tell me if I'm wrong.

With that in mind, the problem is limited to the ES storer and does not require a change to the storer interface or anything else in REST layer. We just need to make sure ES replace the document in the Update method (by using put instead of post under the hood). As a recall, here is what the Storer Update method is supposed to do:

Update replace an item in the backend store by a new version. […]

[muyadan]

Your understanding is correct. ES does not do replace by using update. and it is the intended behavior referenced in documentation.
The reason I requested this pull request is because it is also miss leading in REST layer because the Update is not actually a replace while in the api and docs says that it is a replace.
And in the mode doc says support PUT which is not true either.
http://rest-layer.io/#modes

[rs]

It is true with all other storer. The Update on the storer is supposed to replace. If we just fix the ES issue, all the documented features should work as expected. Did I miss something?

[rs]

The reason it is misleading for you is that we use the mongodb terminology. With mongo, the way to replace a doc is using the Update method with a full document.

[smyrman]

I agree with @muyadan in that Replace would be a less confusing name than Update. I understand that Update is the terminology that MongoDB use for replace, but in REST API documentations, I believe there is often made a distinction.

If we don't need both Update and Replace, one solution could be to rename the existing end-points to Replace, and "deprecate" Update by stating so in the documentation of rest-layer. That said, I am not a big fan of breaking changes for the sake of breaking changes, so it's possible to wait with this rename and/or maybe see if there is a use-case to support both an Update (with PATCH semantics) and a Replace (with PUT semantics), e.g. performance related.

[rs]

If we think Replace would be a better, less confusing name long term, we should change it now rather than later.

[smyrman]

I think it's easier to understand if it's called Replace.

MongoDB uses quite a bit of documentation real-estate in their docs to explain how update either does a partial update of individual fields, or a replace if there are now special "$set" operators in the passed in "payload":

• https://docs.mongodb.com/manual/reference/method/db.collection.update/#update-parameter

As long as we only aim to implement the complete replace semantics through this one method, it's probably better to rename it.

[rs]

So be it.

[smyrman]

@muyadan, I think we need to apologise for slow handling of this PR.

Would you be interested in changing you PRs to instead rename the Update method to Replace as well as to fix/replace it for the rest-layer-es Storage backend?

[smyrman]

As for backwards-compatibility, it's probably OK not to worry about it too much. rest-layer has by definition not yet reached maturity.

If it doesn't add to much complexity, we could optionally add some functionality in the rest-layer repo only so that it looks for Update if Replace don't exist. This might prevent some issues for those who use third-party Storage providers.

[rs]

How could we do that? If we add Update to the Storer interface, those older storer would not implement this interface anymore.

[smyrman]

How could we do that? If we add Update to the Storer interface, those older storer would not implement this interface anymore.

I take it you mean if we rename Update to Replace in the Storer interface, how can we retain backwards compatibility?

The thing is that we can't retain backwards-compatibility here without adding complexity to the public API, so maybe it's best to get some input from people who are maintaining third-party storage handers.

@ajcrowe, @jxstanford: you have implemented rest-layer Storage backends on GitHub. If we are to rename Update to Replace in the Storer interface, how do you prefer that we handle bakckwards-compatibilty?

A) We don't.
B) Through tagged (semantic) versions in Git.
C) Retain (temporary?) backwards-compatibility on the master branch through slightly complicating the API.

As an FYI: we are probably going to break backwards-compatibility again a few times anyway, e.g. in rs/rest-layer#77, so we should not spend too much time on it.

[smyrman]

As for a more firm suggestion of how C (backwards compatibility on the master branch) in the previous post could be implemented:

Remove Update from interface

• We remove Update from the Storer interface
• We add two new interfaces:

type LegacyReplaceStorer interface {
        Storer
        Update(ctx context.Context, item *resource.Item, original *resource.Item) error
}
type ReplaceStorer interface {
        Storer
        Replace(ctx context.Context, item *resource.Item, original *resource.Item) error
}

• We add a private legacyStorerWrapper type that wrapps a LegacyReplaceStorer and implementes ReplaceStorer.

This might sound horrible... There are a few more ways we could do it. E.g. if there is any benefit to allow usage of read-only Storere implementations, we colud move all Write/Delete operations to the new interfaces and name then accordingly as ReadWriteStorere / LegacyReadWriteStorer.

[ajcrowe]

I would probably; given are as @smyrman mentioned not dealing with a mature project, be in favour of a clean change.

[rs]

I'm closing this issue. The problem will be dealt int rs/rest-layer#117.