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

Add PATCH-processing functionality to Resource. #91

Merged
merged 1 commit into from
Feb 2, 2016

Conversation

patrickt
Copy link
Contributor

This extends the decision tree with actions to take in case of a PATCH
request. If one is encountered, the processPatch member is invoked: if
its operation effected no change, it returns a 304 Not Modified; if it
did, return a 204 No Content. This is all invoked directly after testing for
POST.

else o17 r

o17 r@Resource{..} = do
trace "o17"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an updated graph now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charleso Sure thing; anyone have an OmniGraffle doc for it? 😄

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious that web machine doesn't appear to support PATCH right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmcgilchrist Apparently webmachine is implemented based strictly on RFC 2616, which does not discuss HTTP PATCH

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice at some point to tweak for-GET and have the (slightly augmented) airship flow graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm… though for-GET is gorgeous, it departs pretty significantly from the Airship graph, AFAICT.

@patrickt
Copy link
Contributor Author

Ugh, sorry for all the whitespace changes. If you really object, I'll disable stylish-haskell (though it's my contention that everyone should have stylish-haskell on everywhere, with editor integration)

@patrickt
Copy link
Contributor Author

Fixes #90.

@charleso
Copy link
Contributor

Ugh, sorry for all the whitespace changes.

I'm all for consistent styles, whatever they are.

Definitely bikeshedding, but (EDIT: our) style guides are more aligned with something like elm. So we don't, for example, align the import values to avoid the big-reshuffle if you import something slightly longer (I don't know how stylish-haskell deals with this, so that may be a moot point).

In any case, I'm happy to stick with style-haskell (I don't use it at the moment). You could even go so far to have a pre-commit hook script with instructions for setting up locally to ensure it doesn't get missed. And/or an extra build that fails if something isn't formatted correctly (which might be a little extreme).

@patrickt
Copy link
Contributor Author

@charleso Cool, I'll leave it in. What I like about stylish-haskell is that it doesn't futz with indentation. It mostly makes small but helpful changes (aligning type declarations, etc.)

@tmcgilchrist
Copy link
Owner

@patrickt re whitespace, as long as it's consistent, I care more about the code being correct.
I'm reading back through the PATCH RFC now.

@charleso
Copy link
Contributor

👍

, processPost :: Webmachine m (PostResponse m)
, processPost :: Webmachine m (PostResponse m)
-- | As with 'processPost', but called on PATCH requests.
, processPatch :: Webmachine m Bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, and this might be part of the RFC, but do you normally care about the content-type for PATCH in the same way you do with PUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charleso I'd say yes, though I would imagine most people are passing in application/json.

@tmcgilchrist
Copy link
Owner

I don't think this fully handles PATCH as defined in RFC-5789, specifically the Accept-Patch header https://tools.ietf.org/html/rfc5789#section-3

@tmcgilchrist
Copy link
Owner

Also it looks like you should be calling o14 and dealing with the conflict part?
PATCH is more a special case of put so should behave like that.

EDIT

Some interesting issues raised here on the ruby web machine client. I don't think the linked diagram is quite right but it does show the other areas that need to change to support PATCH
webmachine/webmachine-ruby#109

@patrickt
Copy link
Contributor Author

Another note: from the RFC,

If the Request-URI does not point to an existing resource, the server MAY create a new resource, depending on the patch document type (whether it can logically modify a null resource) and permissions, etc.

This is counterintuitive! We presumably need

allowMissingPatch :: Webmachine m Bool

in the resource type.

EDIT: but we will do this later

@patrickt patrickt force-pushed the feature/process-patch branch 4 times, most recently from 783bfe3 to 6565269 Compare January 27, 2016 17:20
This extends the decision tree with actions to take in case of a PATCH
request. If one is encountered, the processPatch member is invoked: if
its operation effected no change, it returns a 304 Not Modified; if it
did, return a 202 Accepted.

Also updates to 0.4.4.0.
@tmcgilchrist
Copy link
Owner

That last commit, connecting o17 to o20, looks right to me.

I'd think the allowMissingPatch would fit in around m7/m5 somewhere and flow into n11. Does that seem right?

PATCH is a rather tricky verb to get right, it'd be good to have an example of how to do it somewhere in the project.

@patrickt
Copy link
Contributor Author

@tmcgilchrist Re: allowMissingPatch: Correct. We're gonna implement that in a subsequent commit, though.

I can definitely add a PATCH example.

@tmcgilchrist
Copy link
Owner

Cheers, I'm happy with that. 👍 from me

patrickt added a commit that referenced this pull request Feb 2, 2016
Add PATCH-processing functionality to Resource.
@patrickt patrickt merged commit b8707d3 into master Feb 2, 2016
@reiddraper reiddraper deleted the feature/process-patch branch February 2, 2016 17:26
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.

3 participants