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

Improve partial item editing / enabling of Transactions extensions #37

Merged

Conversation

seanmurph
Copy link

Background

My overall goal was to change from a:

POST /collections/<collectionId>/items/<itemId>/edit

to

PATCH /collections/<collectionId>/items/<itemId>

Which was more RESTful and also conforms to the stac-api's Transaction specification: https://github.com/radiantearth/stac-api-spec/tree/master/extensions/transaction

An /edit path is more traditionally used to render a HTML form which eventually will post to a PUT/PATCH endpoint.

What's Includes

I also adjusted aspects of areas I was touching directly:

  • The above route change
  • Ability to easily toggle whether Transaction extension endpoints are available
  • Simple HTTP constant utils introduced
  • Fixed a bug where we required a blank properties param even if not are being modified

Explanation of changes

We have a DIY routing approach inside api.js. Previously it was working with paths only, so I added support for also recognizing the HTTP method when it comes to determining different behavior for identical paths.

I found myself working in several cases with inline string literals for HTTP methods. I'd rather use constants and symbols, so I made a really minimal util to provide this.

I introduced a way to easily toggle the transaction endpoints, driven by an environment variable. I also instructions to the README regarding this.

Finally, I fixed an issue that required callers to specify a blank properties param even if none are being modified. The problem before was the edit endpoint trying to set an updatedAt date inside this object, and if it was nil there'd be an exception.

Testing

The new behavior is triggered during a call to:

PATCH /collections/<collectionId>/items/<itemId>

Which is primarily used to change the collection of an item. Sample bodies include:

{
  "collection": "new-collection"
}

or with other properties

{
  "collection": "new-collection",
  "properties": {
    "some-other-property": true
  }
}
  • properties will always get an updated attribute automatically assigned to the current date.
    • If other attributes are alongside in this object, it's merged with them.
    • If the input tries to provide updated as one of those attributes, ours is overwritten.
    • If no properties object is provided, we default to one with only updated

It's probably also worth testing other http methods on this same route, such as:

GET /collections/<collectionId>/items/<itemId>

Toggling the Transactions extensions

See the note in README, but essentially this is driven by a boolean ENV setting in serverless.yml. Previously we relied on commenting code, and moreover a code commit to change it.

# Background

My overall goal was to change from a:

`POST /collections/<collectionId>/items/<itemId>/edit`

to

`PATCH /collections/<collectionId>/items/<itemId>`

Which was more RESTful and also conforms to the stac-api's Transaction specification: https://github.com/radiantearth/stac-api-spec/tree/master/extensions/transaction

An /edit path is more traditionally used to render a HTML form which eventually will post to a PUT/PATCH endpoint.

# What's Includes

I also adjusted aspects of areas I was touching directly:

- The above route change
- Ability to easily toggle whether Transaction extension endpoints are available
- Simple HTTP constant utils introduced
- Fixed a bug where we required a blank `properties` param even if not are being modified

# Explanation of changes

We have a DIY routing approach inside api.js. Previously it was working with paths only, so I added support for also recognizing the HTTP method when it comes to determining different behavior for identical paths.

I found myself working in several cases with inline string literals for HTTP methods. I'd rather use constants and symbols, so I made a really minimal util to provide this.

I introduced a way to easily toggle the transaction endpoints, driven by an environment variable. I also instructions to the README regarding this.

Finally, I fixed an issue that required callers to specify a blank `properties` param even if none are being modified. The problem before was the edit endpoint trying to set an `updatedAt` date inside this object, and if it was nil there'd be an exception.

# Testing

The new behavior is triggered during a call to:

`PATCH /collections/<collectionId>/items/<itemId>`

Which is primarily used to change the collection of an item. Sample bodies include:

```
{
  "collection": "new-collection"
}
```

or with other properties

```
{
  "collection": "new-collection",
  "properties": {
    "some-other-property": true
  }
}
```

- `properties` will always get an `updated` attribute automatically assigned to the current date.
  - If other attributes are alongside in this object, it's merged with them.
  - If the input tries to provide updated as one of those attributes, ours is overwritten.
  - If no properties object is provided, we default to one with only `updated`

-----

It's probably also worth testing other http methods on this same route, such as:

`GET /collections/<collectionId>/items/<itemId>`

# Toggling the Transactions extensions

See the note in README, but essentially this is driven by a boolean ENV setting in serverless.yml.  Previously we relied on commenting code, and moreover a code commit to change it.
@seanmurph seanmurph changed the base branch from master to develop April 21, 2020 19:38
@matthewhanson
Copy link
Member

This is great @seanmurph , thanks so much for this, I'm merging this to develop.

I have to make a rather big change in the coming days which is going to impact that. Right now the primary key in elasticsearch is the ID of the STAC Item. However, this can cause a problem when you reuse the same ID in multiple collections, they need to be separate STAC Items.

So I've got to change the ES primary key to be collectionId + itemId, which will impact how the transaction extension updates an item IF one of the fields to be updated is the collectionId (or the itemId).

So I think if one of the IDs is being changed then this should:

  • get the old Item, merge the new provided fields with it
  • delete the old Item
  • add the new Item

any thoughts on this?

@seanmurph
Copy link
Author

This is great @seanmurph , thanks so much for this, I'm merging this to develop.

My pleasure! So glad it worked out. Great to get involved here.

So I've got to change the ES primary key to be collectionId + itemId, which will impact how the transaction extension updates an item IF one of the fields to be updated is the collectionId (or the itemId).

So I think if one of the IDs is being changed then this should:

get the old Item, merge the new provided fields with it
delete the old Item
add the new Item

Ah I see. Yes, unless we can change the primary key in place (which I'm not sure is too common to do in ES or DBs) I think this is our best choice.

If anything else comes to mind I'll let you know!

@seanmurph
Copy link
Author

Also hopefully there are no side effects with a delete and create (for instance notifications or triggers that happen) that would be confusing for them to occur during an update.

@matthewhanson
Copy link
Member

yeah, if you did have some sort of notification though that would make sense if you changed the collection, you are effectively adding a new item to a collection.

I think the process actually should be add the new item first THEN delete the old item

@seanmurph
Copy link
Author

That sounds good to me!

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

Successfully merging this pull request may close these issues.

2 participants