search

haskell-servant / servant

Main repository for the servant libraries — DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.8k stars 407 forks source link

Remove binary data test case #1679

Closed jkarni closed 1 year ago

jkarni commented 1 year ago

And add changelog entry.

tchoutri commented 1 year ago

Is there any other place where the feature is advertised that we should amend? Doesn't this effectively requires a major version bump?

jkarni commented 1 year ago

@tchoutri it is indeed a major change. Is there a way of indicating that in the changelog.d entry? I haven't been able to find documentation for it.

It doesn't look like the feature is advertised elsewhere.

tchoutri commented 1 year ago

I don't think we have something to advertise such things as breaking changes, but perhaps @alpmestan knows?

ysangkok commented 1 year ago

Have there been other breaking changes since v0.19.1? If not, I think it would be best to undo #1597 and make a release v0.19.2 first. Then, we should think about to resolve this properly, since these APIs seem confusing to me, and it wouldn't be ideal to make a v0.20 only to find out shortly after that there is another shortcoming. Even if the new API implied by #1597 turns out to be optimal, many users still on Servant v0.19 would probably appreciate to be able to enjoy the fixes that were already merged.

ysangkok commented 1 year ago

Have there been other breaking changes since v0.19.1?

I've been digging a bit, and at least according to PVP there have been a couple of breaking changes since v0.19.1. I do think they are unlikely to break anybody in practice, but as far as I understand the PVP, any added type (including synonyms) are automatically breaking.

But I am more worried that somebody is relying on the previous behaviour. Though I will trust your judgement @jkarni . So is this ready for merging? It would be nice to get passing tests on master such that it becomes easier to review PRs.

jkarni commented 1 year ago

But I am more worried that somebody is relying on the previous behaviour. Though I will trust your judgement @jkarni .

It seems like people were relying on the previous previous behavior too, so I don't know a nice way out of this. My general sense is that query params are not a good place for binary data for other reasons too (length limit, NUL can't be there).

I honestly don't know the best way out here, partly because I've just been shepherding other people's PRs and issues rather than making them myself, but also partly because it seems like no solution is perfect. Maybe @lagunoff can chime in.

But definitely, either this PR should be merged or the previous changes reverted. I don't know how exactly I let a failing commit slip in - sorry!

lagunoff commented 1 year ago

Hi guys I don't think I can give any valuable help here, looks like whether to keep or undo https://github.com/haskell-servant/servant/pull/1597 depends on estimation how much people rely on previous behavior and how much damage the change can inflict on the users. Someone has to make this decision I'm ok with any conclusion

tchoutri commented 1 year ago

@jkarni @ysangkok We should probably post something on the Discourse to gather feedback from users.

ysangkok commented 1 year ago

We could also just ask @haskell-servant/maintainers first: Do you agree with removing this test for binary round tripping in QueryParams? If we remove the test, do you think we should have a new major release because of #1551, #1597 and #1641, or should we treat them as bug fixes and release v0.19.2 after merging this PR?

tomsmalley commented 1 year ago

Related issue: https://github.com/haskell-servant/servant/issues/1626

Looks like we have the upstream changes we need to fix this now: https://github.com/fizruk/http-api-data/pull/120

ysangkok commented 1 year ago

@tomsmalley The definition of the new toEncodedQueryParam in http-api-data-0.5.1 is identical to what was added in #1597. Using toEncodedQueryParam would require raising the minimum bound of http-api-data to require at least v0.5.1. Do you think that would be better than what we currently have?

ysangkok commented 1 year ago

Given that https://github.com/haskell-servant/servant/pull/1661 is also a breaking change, how would you feel about merging this and the other pending PRs and then doing a new major release?

If there is no response, I'll go ahead and merge this and #1661 and #1680 on Monday evening CEST.

tchoutri commented 1 year ago

+1 from me. @alpmestan any comment?