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

Define a semantics for "side-deleting" #18

Closed
tchak opened this issue May 5, 2013 · 17 comments
Closed

Define a semantics for "side-deleting" #18

tchak opened this issue May 5, 2013 · 17 comments
Labels

Comments

@tchak
Copy link

tchak commented May 5, 2013

see emberjs/data#379

@hjdivad
Copy link

hjdivad commented May 5, 2013

Personally I'm in favour of having some names within meta reserved for purposes such as this. Conforming servers could then provide something like the following:

// id-style
meta: {
  deleted: {
    posts: [1,2,3],
    people: [1,2]
  }
}

// url-style
meta: {
  deleted: {
    posts: ["http://example.com/posts/1", "http://example.com/posts/2", "http://example.com/posts/3"],
    people: ["http://example.com/people/1", "http://example.com/people/2"]
  }
}

// url-style with templates
meta: {
  urls: {
    posts: "http://example.com/posts/{posts.id}",
    people: "http://example.com/people/{people.id}"
  },
  deleted: {
    posts: [1,2,3],
    people: [1,2]
  }
}

@tchak
Copy link
Author

tchak commented May 5, 2013

Tell me guys if I am out of my mind, but would it be possible to return a json-patch format from server?

@steveklabnik
Copy link
Contributor

Nope, that is 100% possible.

@lukfugl
Copy link

lukfugl commented May 5, 2013

One issue with a json-patch format (as much as I'd like it) is the path property of the returned operations. For the path to have meaning, it has to be interpreted in the context of a particular document.

One choice for this document is the primary document that would have been returned for a GET at the now-deleted resource's URL just prior to the deletion. This would be similar to the implicit document used when POSTing a json-patch sequence to the resource.

But what if the client hadn't already fetched that, and now can't because it's deleted? The client is unable to interpret the path, and doesn't know what related resource was deleted.

Just using explicit collections (keys under deleted) and identifiers (lists of ids or urls under those keys) as suggested by @hjdivad seems appropriate.

@lukfugl
Copy link

lukfugl commented May 5, 2013

I suppose an alternative is for the spec to define a virtual master document, never (at least not required to be) instantiated. This document would need to contain all resources in the system in their respective collections, but with the collections represented as objects with resource identifiers (ids or urls) as members, rather than being lists. So e.g. in our example above the master document would contain, among other things:

{
  posts: {
    1: { ... },
    2: { ... },
    3: { ... },
    ...
  },
  people: {
    1: { ... },
    2: { ... },
    ...
  },
  ...
}

Given that master document, the side-deletes could be represented as a json-patch sequence interpreted in the context of that document:

meta: {
  patches: [
    { op: "remove", path: "/posts/1" },
    { op: "remove", path: "/posts/2" },
    { op: "remove", path: "/posts/3" },
    { op: "remove", path: "/people/1" },
    { op: "remove", path: "/people/2" },
  ]
}

I feel like this just complicates the spec (introduction of the "master document") while still repeating the same information from @hjdivad's proposal (now embedded in the meta.patches[i].path).

@paddycarver
Copy link

If I'm reading this right, we're trying to give the server a way to inform the client if the server knows about things that have been deleted and it thinks the client doesn't know they're deleted yet. So any request at all could return this deleted information.

That sounds a lot like it's violating the whole statelessness aspect. Am I missing something? This sounds... incredibly fragile.

@lukfugl
Copy link

lukfugl commented May 7, 2013

I think that's where it started and the angle that many examples (including mine) came from. But I agree with you there; we should keep client state (what changes on the server doesn't the client know about yet?) away from the server. See #34 for a discussion around ways we might handle app staleness without client state on the server. So yeah, I recommend removing that portion of this discussion here.

However, there may still be cases where a PUT, POST, or DELETE request targeted at one resource may trigger immediate changes in other resources -- changes that would be of interest to the client. I think we can continue this discussion centered on how to best communicate (or if we even should... I'm not certain we should) those changes to the client in the HTTP response.

@hjdivad
Copy link

hjdivad commented May 7, 2013

@paddyforan No, absolutely not. Having the server understand client state is a recipe for sadness. In fact I don't think that's where the discussion even started. @tchak's referenced ember-data pull-request exists for the server to inform clients of documents deleted as a result of the request.

A common case is a DELETE /resource response informing clients of related documents that were destroyed.

@paddycarver
Copy link

Ah, so DELETE /users/me happens to remove all my posts, and we want the server to be able to say "These posts were all deleted as a side-effect of this request"?

Sorry for the confusion. That sounds totally reasonable. Ignore me.

@heartsentwined
Copy link

@tchak I'm not sure if this is a good idea.

I am okay with making the server returning a list of deleted records, or clearing the data store altogether. However, I would lean towards the latter. The former is an expensive calculation server-side, essentially requiring it to diff all requests from all users; what's more, that's a diff that relies on a database operation (hence slow).

Directly quoted from my own comment in emberjs/data#962

@hjdivad
Copy link

hjdivad commented May 14, 2013

@heartsentwined I think the only use case we're discussing here is a server responding with records that have been deleted as a result of the request (although nothing prevents the server from informing clients of other deleted records). This is unlikely to be expensive, as the server has to compute these records anyway to figure out who to delete.

Remember that jsonapi is about having a standard way of handling the common use cases, and a well understood extension mechanism (eg meta). Nothing would prevent clients and servers from supporting a clearEntireStore token in meta.

@tchak
Copy link
Author

tchak commented May 14, 2013

@heartsentwined Well a query to get deleted records could be as simple as :
SELECT id FROM posts WHERE updated_at > since AND deleted_at != NULL
I would not call it expensive.

@heartsentwined
Copy link

@hjdivad @tchak well what I mean is that, while a single query would not be expensive as @tchak pointed out, adding this to every single request would be rather overkill for me. The expensive part is that they can never be served from a cache. A general GET request that would have generated a query like SELECT * FROM comments WHERE post_id = 1 would normally be served from the cache instead of hitting the db. Implementing the side-deleting-response would add a non-cacheable call to all requests, which doubles the work at every request, and which hits the db every single time.

@tchak
Copy link
Author

tchak commented May 15, 2013

@heartsentwined agreed. We newer talked about adding it to every request. In my case I use it with since token and I can imagine the use case on CUD operations. All this operations will probably hit the db anyway.

@dgeb
Copy link
Member

dgeb commented Feb 19, 2015

I would recommend using the Patch extension for performing multiple operations in a single request.

@dgeb dgeb closed this as completed Feb 19, 2015
@tchak
Copy link
Author

tchak commented Feb 19, 2015

@dgeb I am not sure I seen examples of responses with json-patch format in the current documentation. Only requests. Should we add some ?

@dgeb
Copy link
Member

dgeb commented Feb 19, 2015

@tchak Agreed - I just opened #351 to track the need for this.

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

No branches or pull requests

7 participants