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 Content hooks #2267

Merged
merged 2 commits into from Apr 15, 2020
Merged

Add Content hooks #2267

merged 2 commits into from Apr 15, 2020

Conversation

grosch
Copy link
Contributor

@grosch grosch commented Mar 26, 2020

Adds optional beforeEncode and afterDecode methods to Content protocol (#2267).

Docs: https://docs.vapor.codes/4.0/content/#hooks

@grosch grosch requested a review from tanner0101 March 26, 2020 17:18
@grosch grosch added the enhancement New feature or request label Mar 26, 2020
@grosch grosch requested a review from gwynne March 26, 2020 18:32
@tanner0101 tanner0101 added this to Awaiting Review in Vapor 4 via automation Mar 26, 2020
@Joannis
Copy link
Member

Joannis commented Apr 3, 2020

I think the potential of this PR is limited because most of the time I need to sanitize data, I need to do so in the context of a Request.

@grosch
Copy link
Contributor Author

grosch commented Apr 3, 2020

Why does that make it of limited use? That's the exact use case this is for. Every request that I receive will now be automatically sanitized, and every time I create a DTO, to send back, it'll be sanitized. I don't have to manually call it any longer, and I can't forget to call it.

@Joannis
Copy link
Member

Joannis commented Apr 3, 2020

The limitation is in the lack of context available within this function. Either it's part of the Content conformant type, or you don't have access.

@Joannis
Copy link
Member

Joannis commented Apr 3, 2020

You could make the calls look like this:

mutating func beforeEncode(into response: Response, for request: Request) throws
mutating func afterDecode(into response: Response, for request: Request) throws

That way you can know what user is logged in, from the Request. Either through the headers (token) or through the associated userInfo Although response won't be useful for many people, it's still an important piece of context.

@Joannis
Copy link
Member

Joannis commented Apr 3, 2020

One more note; I believe this to be a very subjective PR. So I can see it being delayed until after the Vapor 4 release. But it's such a common use case that I personally do feel that it deserves to be in the core framework in some form.

@gwynne
Copy link
Member

gwynne commented Apr 3, 2020

Doesn't Validation cover at least the afterDecode() part of this?

@grosch
Copy link
Contributor Author

grosch commented Apr 3, 2020

On the afterDecode I wouldn't want to pass a response. The decode of this might have nothing to do with a response. You're simply fixing up the input that was sent to you. I can see the two being useful in the beforeEncode parameter.

@grosch
Copy link
Contributor Author

grosch commented Apr 3, 2020

@gwynne No, because the intent of this is to modify the data itself.

@Joannis What you asked for isn't actually possible because most places the encode or decode would be called doesn't have a reference to either the request or the result

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

👍 This looks good to me. Could you add a section to https://github.com/vapor/docs/blob/master/4.0/docs/content.md explaining how to use this new feature? I'd like to merge docs + new API at the same time so we can include a link in the release notes.

Sources/Vapor/Content/Content.swift Show resolved Hide resolved
@tanner0101 tanner0101 added the semver-minor Contains new API label Apr 15, 2020
@tanner0101 tanner0101 moved this from Awaiting Review to Awaiting Updates in Vapor 4 Apr 15, 2020
@tanner0101
Copy link
Member

Docs PR: vapor/docs#471

@tanner0101 tanner0101 changed the title Adds automatic calls to beforeEncode() and afterDecode() on Content Add Content hooks Apr 15, 2020
@tanner0101 tanner0101 merged commit 2698e2c into master Apr 15, 2020
Vapor 4 automation moved this from Awaiting Updates to Done Apr 15, 2020
@tanner0101 tanner0101 deleted the sanitize branch April 15, 2020 20:20
@tanner0101
Copy link
Member

These changes are now available in 4.2.0

@tanner0101
Copy link
Member

These changes are now available in 4.3.0

pull bot pushed a commit to scope-demo/vapor that referenced this pull request Apr 15, 2020
* Adds automatic calls to beforeEncode() and afterDecode() on Content

* Added doc comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new API
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants