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

On fetched event hooks for viewing all versions or version diffs #475

Closed
cuibonobo opened this issue Oct 11, 2014 · 7 comments
Closed

On fetched event hooks for viewing all versions or version diffs #475

cuibonobo opened this issue Oct 11, 2014 · 7 comments

Comments

@cuibonobo
Copy link

There's currently a TODO listed in the getitem method that is preventing requests with ?version=all or ?version=diffs from executing on_fetched events. (See here.) The project I'm working on uses event hooks extensively to examine the content of a document before sending it, so I'm particularly interested what needs to be done to get this working.

For my project I just commented out the if statement that's preventing the event hooks from firing and everything seems to be working fine. What else is missing?

/cc @joshvillbrandt because I know document versioning is his baby.

@joshvillbrandt
Copy link
Contributor

At the very least, the portion of code would have to register the callbacks through a for loop for ?version=all. I can't remember if there was something deeper that I was trying to protect against or not... We should probably still prevent ?version=diffs since those won't even be completed documents and most users' hook functions wouldn't export or support partial documents. Do you need this to work for diffs as well? I suppose it could be a configuration option to allow diffs through, that way the developer would make a conscience choice to have the hook function be ready for partial documents.

@jenmontes - want to create a pull request that adds a for loop for ?version=all but still prevents ?version=diffs?

@cuibonobo
Copy link
Author

I actually came to the same conclusion as you and decided to still exclude ?version=diffs because of the partial document problem. The for loop is a good idea. I was just letting my event hook handle it but putting it right in getitem makes sense.

I'll submit a pull request for this.

@cuibonobo
Copy link
Author

While I was working on this, I think I hit on what may have given you pause before: if HATEOAS is turned on, a normal getitem response includes _links in its body, whereas in a ?version=all request _links is returned alongside _items instead.

To illustrate this more plainly, here's a normal GET:

{
    ...
    "_links": {
        "collection": {
            "href": "/posts", 
            "title": "posts"
        }, 
        "parent": {
            "href": "/", 
            "title": "home"
        }, 
        "self": {
            "href": "/posts/1ouph3", 
            "title": "post"
        }
    },
    "_version": 2,
    "content": {
        "text": "OooooOOoooOoh!!"
    }
}

And here's a GET with ?version=all:

{
    "_items": [
        {
            ...
            "_version": 1,
            "content": {
                "text": "Hello, world!"
            }
        }, 
        {
            ...
            "_version": 2,
            "content": {
                "text": "OooooOOoooOoh!!"
            }
        }
    ], 
    "_links": {
        "collection": {
            "href": "/posts", 
            "title": "posts"
        }, 
        "parent": {
            "href": "/", 
            "title": "home"
        }
    }
}

So essentially, if HATEOAS is turned on, on_fetched events might have an expectation to find _links in the body of the document but it won't be there for ?version=all requests.

I'm at a loss for what to do about this. Add _links to the item body just before the on_fetched callbacks and then strip it back out when the callbacks are done?

@cuibonobo
Copy link
Author

After looking at the response of ?version=all more closely I'm wondering if it would be more correct to return something like this:

{
    "_items": [
        {
            ...
            "_links": {
                "collection": {
                    "href": "/posts?version=all", 
                    "title": "posts"
                }, 
                "parent": {
                    "href": "/posts", 
                    "title": "home"
                },
                "self": {
                    "href": "/posts/1ouph3?version=1", 
                    "title": "post"
                }
            },
            "_version": 1,
            "content": {
                "text": "Hello, world!"
            }
        }, 
        {
            ...
            "_links": {
                "collection": {
                    "href": "/posts?version=all", 
                    "title": "posts"
                }, 
                "parent": {
                    "href": "/posts", 
                    "title": "home"
                },
                "self": {
                    "href": "/posts/1ouph3?version=2", 
                    "title": "post"
                }
            },
            "_version": 2,
            "content": {
                "text": "OooooOOoooOoh!!"
            }
        }
    ], 
    "_links": {
        "collection": {
            "href": "/posts", 
            "title": "posts"
        }, 
        "parent": {
            "href": "/", 
            "title": "home"
        },
        "self": {
            "href": "/posts/1ouph3?version=all", 
            "title": "post"
        }
    }
}

I'll be the first to admit that it's pretty verbose but the whole point of HATEOAS is to not have to depend on out-of-band knowledge to be able to navigate the API structure.

@joshvillbrandt
Copy link
Contributor

Agreed on pretty much all accounts.

It seems like your most recent suggestion (_links in all of the sub-documents) is most correct, but yeah, it is also verbose. I don't really want all of that in my output, but I guess that is because I'm not using HATEOAS in my application even though I have it turned on. (I think it makes a response more human readable too which is why I have it left on.)

I'm game for really any option here. (Except for adding _links before a hook and removing it directly afterwards...that's just silly.) Anyway, pick your favorite approach and go for it.

@cuibonobo
Copy link
Author

In the end I went for a middle-ground approach where self links in ?version=all requests will display a link to a particular version number, but I'm putting collection and parent links only on the root _links object.

Example:

{
    "_items": [
        {
            ...
            "_links": {
                "self": {
                    "href": "/posts/1ouph3?version=1", 
                    "title": "post"
                }
            },
            "_version": 1,
            "content": {
                "text": "Hello, world!"
            }
        }, 
        {
            ...
            "_links": {
                "self": {
                    "href": "/posts/1ouph3?version=2", 
                    "title": "post"
                }
            },
            "_version": 2,
            "content": {
                "text": "OooooOOoooOoh!!"
            }
        }
    ], 
    "_links": {
        "collection": {
            "href": "/posts/1ouph3", 
            "title": "post"
        }, 
        "parent": {
            "href": "/posts", 
            "title": "posts"
        }, 
        "self": {
            "href": "/posts/1ouph3?version=all", 
            "title": "post"
        }
    }
}

@joshvillbrandt
Copy link
Contributor

This is the same HATEOAS response as on normal collection endpoints, right? So that seems good.

👍

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

No branches or pull requests

2 participants