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

feat: adds webdav methods that require body & content type parsing #5411

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

johaven
Copy link
Contributor

@johaven johaven commented Apr 20, 2024

Adds webdav methods that require parsing, missing from this PR: #3836

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Can you add some tests for them?

@johaven
Copy link
Contributor Author

johaven commented Apr 21, 2024

These methods are already tested in the PR I mentioned

@metcoder95
Copy link
Member

Seems lint is failing

@gurgunday

This comment was marked as off-topic.

@johaven

This comment was marked as off-topic.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

A test is needed

@gurgunday
Copy link
Member

Oh this PR is fine, my mistake

I meant to talk about this issue where you suggested backporting #4409, that would be breaking

@johaven johaven requested a review from mcollina April 21, 2024 18:07
@gurgunday
Copy link
Member

The modified tests simply changed the execution at: https://github.com/fastify/fastify/pull/5411/files#diff-15a9e086692377c86b02752c54c928f4ca6f697758fc5dacc09bbde1d2dff96bR32-R45

Not sure if coverage is enabled but now the case where content-type is undefined may not be getting tested, so I think it would be better to add these as separate tests; just add one more request to each test with the header

@@ -42,7 +42,7 @@ fastify.route(options)
[here](./Validation-and-Serialization.md) for more info.

* `body`: validates the body of the request if it is a POST, PUT, PATCH,
TRACE, or SEARCH method.
TRACE, SEARCH, PROPFIND, PROPPATCH, LOCK or UNLOCK method.
Copy link
Member

Choose a reason for hiding this comment

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

And also not familiar with webdav but just pointing out that UNLOCK method wasn't added anywhere if that was unintentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a check in the webdav RFC, UNLOCK method does not require a body, a mistake fixed in 54bf25c

I forgot to update the doc, sorry !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in d59e594

@johaven
Copy link
Contributor Author

johaven commented Apr 21, 2024

The modified tests simply changed the execution at: https://github.com/fastify/fastify/pull/5411/files#diff-15a9e086692377c86b02752c54c928f4ca6f697758fc5dacc09bbde1d2dff96bR32-R45

Not sure if coverage is enabled but now the case where content-type is undefined may not be getting tested, so I think it would be better to add these as separate tests; just add one more request to each test with the header

Done in 97ff092

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@gurgunday
Copy link
Member

gurgunday commented Apr 21, 2024

Just a note: tests were modified, which means behavior has changed, but these methods work with a body, and it really seems like an oversight from the original PR that added them, so I take it as a bugfix

@johaven
Copy link
Contributor Author

johaven commented Apr 21, 2024

Just a note: tests were modified, which means behavior has changed, but these methods work with a body, and it really seems like an oversight from the original PR that added them, so I take it as a bugfix

It depends on the methods concerned but the RFC is rather flexible as you can see here (https://datatracker.ietf.org/doc/html/rfc4918#section-9.1) :

A client may choose not to submit a request body.  An empty PROPFIND
  request body MUST be treated as if it were an 'allprop' request.

Overall I agree with you, I think that indeed the tests were not complete but part of the implementation was missing.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 69dcea1 into fastify:main Apr 22, 2024
39 checks passed
@johaven johaven deleted the feat-webdav-methods-body branch April 23, 2024 09: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.

None yet

5 participants