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 replaceOrAddHeader function to Yesod.Core.Handler module #1417

Merged
merged 13 commits into from Jul 28, 2017

Conversation

Projects
None yet
3 participants
@psibi
Member

psibi commented Jul 13, 2017

Fixes #1416

psibi added some commits Jul 13, 2017

Show outdated Hide outdated yesod-core/Yesod/Core/Handler.hs
replaceHeader :: Header -> Endo [Header] -> Endo [Header]
replaceHeader header endo =
let allHeaders :: [Header] = appEndo endo []
in Endo

This comment has been minimized.

@snoyberg

snoyberg Jul 13, 2017

Member

The structure here looks wrong, you're adding the new header to the front of the list, then any additional headers added after, and finally the previously defined header. Instead, I think you want:

Endo $ \rest -> header : filter (\x -> not (sameHeaderName x header)) ++ rest

We could optimize it by fusing the filter and ++ calls, but I don't think it's necessary.

@snoyberg

snoyberg Jul 13, 2017

Member

The structure here looks wrong, you're adding the new header to the front of the list, then any additional headers added after, and finally the previously defined header. Instead, I think you want:

Endo $ \rest -> header : filter (\x -> not (sameHeaderName x header)) ++ rest

We could optimize it by fusing the filter and ++ calls, but I don't think it's necessary.

This comment has been minimized.

@psibi

psibi Jul 13, 2017

Member

@snoyberg

The structure here looks wrong, you're adding the new header to the front of the list, then any additional headers added after, and finally the previously defined header.

I was under the impression that the ordering didn't matter. I have updated the code to fix it and have added additional test to ensure that any header isn't lost because of the above function.

@psibi

psibi Jul 13, 2017

Member

@snoyberg

The structure here looks wrong, you're adding the new header to the front of the list, then any additional headers added after, and finally the previously defined header.

I was under the impression that the ordering didn't matter. I have updated the code to fix it and have added additional test to ensure that any header isn't lost because of the above function.

This comment has been minimized.

@paul-rouse

paul-rouse Jul 13, 2017

Contributor

Hmm.. It looks as if the ordering is still wrong in your latest repo. I am trying this

addHeader "Foo" "Bar"
replaceOrAddHeader "Foo" "Baz"
addHeader "Foo" "Dup"
replaceOrAddHeader "FOO" "Final"

and I get

FOO: Final
Foo: Baz
Vary: Accept, Accept-Language
Foo: Dup

I did have a quick look at this earlier in the week, and I ended up with a modified version of tell which also takes a filtering function alter wrapped up in Endo:

alterAndTell :: MonadHandler m => Endo [Header] -> Endo [Header] -> m ()
alterAndTell alter hs = modify f
  where f g = let altered = Endo (appEndo (alter `mappend` ghsHeaders g) [] ++)
              in g { ghsHeaders = altered `mappend` hs }
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

Hmm.. It looks as if the ordering is still wrong in your latest repo. I am trying this

addHeader "Foo" "Bar"
replaceOrAddHeader "Foo" "Baz"
addHeader "Foo" "Dup"
replaceOrAddHeader "FOO" "Final"

and I get

FOO: Final
Foo: Baz
Vary: Accept, Accept-Language
Foo: Dup

I did have a quick look at this earlier in the week, and I ended up with a modified version of tell which also takes a filtering function alter wrapped up in Endo:

alterAndTell :: MonadHandler m => Endo [Header] -> Endo [Header] -> m ()
alterAndTell alter hs = modify f
  where f g = let altered = Endo (appEndo (alter `mappend` ghsHeaders g) [] ++)
              in g { ghsHeaders = altered `mappend` hs }

This comment has been minimized.

@psibi

psibi Jul 13, 2017

Member

@paul-rouse So, this should be the correct ordering - right ?

FOO: Final
Foo: Dup
Foo: Baz
@psibi

psibi Jul 13, 2017

Member

@paul-rouse So, this should be the correct ordering - right ?

FOO: Final
Foo: Dup
Foo: Baz

This comment has been minimized.

@paul-rouse

paul-rouse Jul 13, 2017

Contributor

@psibi I don't think so. The behaviour of addHeader is that a later call puts the header later in the result, so to be consistent, and also "natural", I think, the result should have been (since Yesod appears to add Vary before the handler code):

Vary: Accept, Accept-Language
Foo: Baz
Foo: Dup
FOO: Final

Obviously that's with the case sensitive match still - actually we want just the Vary and the last FOO, of course!

@paul-rouse

paul-rouse Jul 13, 2017

Contributor

@psibi I don't think so. The behaviour of addHeader is that a later call puts the header later in the result, so to be consistent, and also "natural", I think, the result should have been (since Yesod appears to add Vary before the handler code):

Vary: Accept, Accept-Language
Foo: Baz
Foo: Dup
FOO: Final

Obviously that's with the case sensitive match still - actually we want just the Vary and the last FOO, of course!

This comment has been minimized.

@psibi

psibi Jul 13, 2017

Member

@paul-rouse Thanks for the feedback. I have fixed (hopefully!) the ordering logic with the latest push. With respect to the equality of case insensitive comparison, I will open a new issue.

@psibi

psibi Jul 13, 2017

Member

@paul-rouse Thanks for the feedback. I have fixed (hopefully!) the ordering logic with the latest push. With respect to the equality of case insensitive comparison, I will open a new issue.

psibi added some commits Jul 13, 2017

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

Shouldn't the comparison with existing headers be case-insensitive?

Contributor

paul-rouse commented Jul 13, 2017

Shouldn't the comparison with existing headers be case-insensitive?

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 13, 2017

Member

@paul-rouse You're right. But I think it's better fixed at the Eq instances of Header type rather than here ?

Member

psibi commented Jul 13, 2017

@paul-rouse You're right. But I think it's better fixed at the Eq instances of Header type rather than here ?

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

Wouldn't Eq on Header compare both the header name and the value?

Contributor

paul-rouse commented Jul 13, 2017

Wouldn't Eq on Header compare both the header name and the value?

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 13, 2017

Member

@paul-rouse Maybe I'm misunderstanding something, but can't we control the behavior anyway we like when we write our own manual instances of the type ?

Member

psibi commented Jul 13, 2017

@paul-rouse Maybe I'm misunderstanding something, but can't we control the behavior anyway we like when we write our own manual instances of the type ?

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

Yes, but there's already a derived Eq instance for Header in Types, which I assumed was there for a reason. Perhaps it isn't used - I haven't looked that far!

Contributor

paul-rouse commented Jul 13, 2017

Yes, but there's already a derived Eq instance for Header in Types, which I assumed was there for a reason. Perhaps it isn't used - I haven't looked that far!

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 13, 2017

Member

@paul-rouse Ah okay. My proposal is just to remove the derived Eq instance and write our custom instance. :)

Member

psibi commented Jul 13, 2017

@paul-rouse Ah okay. My proposal is just to remove the derived Eq instance and write our custom instance. :)

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

@psibi Did you also see my comment on the header ordering - I realise I didn't mention you, and I don't think github sends notifications for those.

Contributor

paul-rouse commented Jul 13, 2017

@psibi Did you also see my comment on the header ordering - I realise I didn't mention you, and I don't think github sends notifications for those.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 13, 2017

Member

@paul-rouse Just seeing it and that indeed is an issue. I will comment there.

Member

psibi commented Jul 13, 2017

@paul-rouse Just seeing it and that indeed is an issue. I will comment there.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

@psibi I fear I may have contributed to a miscommunication (in the now obsolete comments). This ordering is fine, but I don't think you necessarily have to keep the replacement in the original position. The example I showed had additions out of order.

Contributor

paul-rouse commented Jul 13, 2017

@psibi I fear I may have contributed to a miscommunication (in the now obsolete comments). This ordering is fine, but I don't think you necessarily have to keep the replacement in the original position. The example I showed had additions out of order.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 13, 2017

Member

@paul-rouse Ah, I was under the impression that you were concerned about the ordering there. :) I will fix the extra additions which you have illustrated in this issue soon: #1418

Member

psibi commented Jul 13, 2017

@paul-rouse Ah, I was under the impression that you were concerned about the ordering there. :) I will fix the extra additions which you have illustrated in this issue soon: #1418

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 13, 2017

Contributor

@psibi I was concerned about the ordering, but only to the extent that when replaceOrAddHeader added a header it was at the front, unlike addHeader. Thinking again, I was probably being too strict, since the header is guaranteed unique after replaceOrAddHeader, and any subsequent addHeader with the same name will still append (which does matter).

Contributor

paul-rouse commented Jul 13, 2017

@psibi I was concerned about the ordering, but only to the extent that when replaceOrAddHeader added a header it was at the front, unlike addHeader. Thinking again, I was probably being too strict, since the header is guaranteed unique after replaceOrAddHeader, and any subsequent addHeader with the same name will still append (which does matter).

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 14, 2017

Member

@paul-rouse With the latest push, it does case insensitive equality check on header name. This is the response we get for your above code right now:

Vary: Accept, Accept-Language
FOO: Final
Member

psibi commented Jul 14, 2017

@paul-rouse With the latest push, it does case insensitive equality check on header name. This is the response we get for your above code right now:

Vary: Accept, Accept-Language
FOO: Final
@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 14, 2017

Contributor

Sounds great, but I'm not seeing a push at the moment :)

Contributor

paul-rouse commented Jul 14, 2017

Sounds great, but I'm not seeing a push at the moment :)

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 14, 2017

Member

@paul-rouse Ah, sorry about that. Pushed it now.

Member

psibi commented Jul 14, 2017

@paul-rouse Ah, sorry about that. Pushed it now.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Jul 14, 2017

Contributor

@psibi Yes that looks great! I have tried a few more combinations of addHandler and replaceOrAddHandler and all is fine. Sorry I was unclear and led us into an over-pedantic view of the ordering, but the order is certainly valid. Thank you!

Contributor

paul-rouse commented Jul 14, 2017

@psibi Yes that looks great! I have tried a few more combinations of addHandler and replaceOrAddHandler and all is fine. Sorry I was unclear and led us into an over-pedantic view of the ordering, but the order is certainly valid. Thank you!

psibi added some commits Jul 28, 2017

Merge remote-tracking branch 'origin/master' into header-yesod
Conflicts resolved in:
	yesod-core/ChangeLog.md
	yesod-core/yesod-core.cabal
@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Jul 28, 2017

Member

@paul-rouse Thanks for the detailed feedback in the PR!

@snoyberg I'm merging this as the Travis test passes and the Windows build seems to be failing for reasons not related to this PR. Can you make a new yesod-core release ?

Member

psibi commented Jul 28, 2017

@paul-rouse Thanks for the detailed feedback in the PR!

@snoyberg I'm merging this as the Travis test passes and the Windows build seems to be failing for reasons not related to this PR. Can you make a new yesod-core release ?

@psibi psibi merged commit e027652 into yesodweb:master Jul 28, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment