-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Refactor App.hs and related changes #1725
Conversation
I only skimmed through it on the surface, so far. Here's a couple of notes. I really like this:
In general, I like that. Although I wonder whether we should use shorter prefixes. At times this almost reads like Java ;) One thing I noted independently of this PR: I think the whole WDYT?
+1 - I had thought about that one before, too.
I really like the flat structure - the deep nesting was hard to follow for me, too.
TBH, everything that stylishhaskell does, I don't care about during development - so I don't find it hard to maintain. I never pad imports manually. I just add them and when I'm done I feel like aligning things often helps with readability, especially in highly repeated patterns like imports, type declarations or pattern matching / case statements. For a couple of months now, I'm aligning things heavily in SQL, too (see #1702 for an example and https://www.sqlstyle.guide/ for the background) and I feel like this really helps readability a lot. For me readability is more important than the additional effort needed to pad things nicely. Coming back at a piece of code after a longer time to change something, I spent the most time reading and understanding what was done before - not padding things after the change. |
Maybe we can do something in the middle, too. For external modules:
For internal modules:
|
Took me a while to get used to it. But now I'm linking the new structure :) (I guess I built compression tolerance and got too used to Since this is a big refactor, one thing I like to see is if this change brings a perf downgrade, doesn't look like it but I want to be certain(maybe fusion matters here). So perhaps run this change with the load tests on #1609 and compare?
How about Prelude/Protolude? I think we can make an exception in this case.
Short prefixes and SQL for the Hasql import sounds good.
Strongly agree on this one. Too verbose. I feel it optimizes too much for the newcomer and makes reading code harder for more experienced postgrest devs.
I remember the main reason for putting things in Also, keep in mind that |
For me, without a haskell background, Prelude/Protolude is about the same as every other external module :D. I'm mostly confused by all the
Ah, thanks for the background on this. Maybe we can still make the cut somewhere else and just put a few types in |
Another thing about Protolude is that symbols would be too verbose prefixed: <$>, <*>, =<<
-- vs
Protolude.<$>, Protolude.<*>, Protolude.=<< Enforcing the qualifier would defeat the purpose to have symbols, I think. I've noted that Hasql uses qualified imports for internal modules, but some of them are short(like Still, I'd prefer not qualifying internal modules. |
Actually I'm finding some functions(like |
I feel like this is more an exception than the norm. For many of the exported functions we currently have, it's very easy to see which module they belong to. This is most prominent with all the field lookup functions for records functions. Just look at this: handleRequest context@(RequestContext _ dbStructure apiRequest _) =
case (ApiRequest.iAction apiRequest, ApiRequest.iTarget apiRequest) of The change I would make here, would be to change the field prefix for handleRequest context@(RequestContext _ dbStructure ApiRequest{..} _) =
case (reqAction, reqTarget) of In my opinion this is muuch quicker to "parse" visually and basically just as explicit, because of the prefix in the field name. |
Agree that some parts have become quite verbose and depart a bit from the typical Haskell style. I think this is a very valuable discussion we are having here, as I think there is a huge value in having the code as easy to follow and accessible as possible. PostgREST is one of the more visible open source projects outside the Haskell ecosystem, and a entryway for some new users of the language (at least it was for me). So let me be the devil's advocate in a few points :-)
I think ideally, we would turn this convention around, avoiding prefixes within modules and using prefixed imports to disambiguate. For example in the Elm ecosystem (another ML language), it's customary to prefix all imported variables (https://elm-lang.org/docs/style-guide). This makes Elm code very easy to follow. It also avoids polluting modules with prefixes internally. So we would at some point use
That looks like a great simplification to make it more readable!
I think
Agree that this should be one exception for wildcard import, as Protolude conceptionally replaces the Prelude that is available by default. At some point we might want to roll our own Prelude, as e.g. Hasql does.
Totally agree that we should split this up. If circular imports are a concern, we could still have PostgREST.Types.DbStructure, PostgREST.Types.ContentTypes etc. separately. |
Thanks for adding the refactor for A couple of notes:
|
Fully agree!
Yes, I thought about this briefly, too. It will however:
Using "proper" prefixes instead of "in-name-prefixes" felt more "proper" for me, too - at first. But now I like the field name prefixes that we have.
"Looking up Bottom line is: Start out with the PostgREST code-base and you'll have to learn some commonly used prefixes used for import. Once you understand we're using
Absolutely, I agree with both of you. It looks like |
860a830
to
b90515c
Compare
test/Feature/SingularSpec.hs
Outdated
@@ -70,8 +70,7 @@ spec = | |||
`shouldRespondWith` | |||
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] | |||
{ matchStatus = 406 | |||
, matchHeaders = [ matchContentTypeSingular | |||
, "Preference-Applied" <:> "tx=commit" ] | |||
, matchHeaders = [ matchContentTypeSingular ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the refactored way we handling errors, errors go straight through optionalRollback
untouched and this header is not being added. I believe this is fine, as the transaction is always rolled back on error and it doesn't matter what the tx
preference was. As it's clear what happened to the user, the header is not needed according to https://tools.ietf.org/html/rfc7240#page-7 section 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wolfgangwalther WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think it's possible to commit a transaction and still return a 4xx
/ 5xx
error code via GUC: https://postgrest.org/en/latest/api.html#setting-response-status-code
This means, that you can not tell from the error code alone whether the transaction was comitted or rolled back.
Is it possible to refactor to avoid this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit about this @wolfgangwalther : It would be relatively simple to always set the preference header (catch any errors, turn them into a response, set the header), but I wonder whether the new behavior that emerged here is more correct.
Setting a custom status via GUC still works and will set the preference header as before, regardless of what the status code is.
Setting the preference header will only be skipped when a true error happened and the transaction will definitely be rolled back. In those cases, showing tx=commit seems misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a custom status via GUC still works and will set the preference header as before, regardless of what the status code is.
Setting the preference header will only be skipped when a true error happened and the transaction will definitely be rolled back. In those cases, showing tx=commit seems misleading.
The whole thing is a bit more complicated, because we have db-tx-end=commit|commit-allow-override|rollback-allow-override|rollback
settings. This creates a lot of different scenarios.
I understand RFC 7240 as follows: We should only ever return a preference that was asked for with Preference-Applied
. This means we should never return a Preference-Applied: tx=rollback
header, when we had a Prefer: tx=commit
header before.
This basically means that we can not use the Preference-Applied
header for feedback about the actual result ("committed" or "rolled back") of the transaction. A couple of examples, where this fails:
Assume an error response with 4xx
status code. With db-tx-end=commit-allow-override
and Prefer: tx=commit
: Setting a Preference-Applied: tx=rollback
seems wrong. How would we differ between the "error via RAISE" and "error via GUC" cases then? We could only return nothing for RAISE (i.e. a true rollback) and Preference-Applied: tx=commit
for the GUC case (i.e. a true commit). This basically assumes that the transaction was rolled back for any 4xx
error code by default. But now assume we have db-tx-end=commit
and Prefer: tx=rollback
. For a GUC error response this results in a commit in the end, because the override is not allowed. But we can't return Preference-Applied: tx=commit
- this was not asked for. And we can't assume a rollback by default either, because that wouldn't be true.
In short: We can only use the Preference-Applied
header to indicate what our transaction termination command was (COMMIT
or ROLLBACK
) - completely independent of whether the transaction was implicitly rolled back before because of an error. Note, that this is also what is implied by the config option db-tx-end
.
There are a couple of other reasons why it makes a lot of sense to return a Preference-Applied
header whenever we understand and accept that preference - regardless of whether it might be sensible to assume this behaviour by default in that specific case. See #740.
The current behaviour should be kept. Our goal should be to respond with more Preference-Applied
headers instead of less.
e331545
to
e7ecea7
Compare
@steve-chavez There will be conflicts with #1729, let's get that PR merged first and then I'll rebase/backport the changes here |
@@ -42,7 +42,7 @@ import qualified Data.Map.Strict as M | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't comment on the hidden line above (the Map
import). If that is still used after the split-up into CLI
, the same reasoning for naming etc. applies as I commented there.
test/Feature/SingularSpec.hs
Outdated
@@ -70,8 +70,7 @@ spec = | |||
`shouldRespondWith` | |||
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] | |||
{ matchStatus = 406 | |||
, matchHeaders = [ matchContentTypeSingular | |||
, "Preference-Applied" <:> "tx=commit" ] | |||
, matchHeaders = [ matchContentTypeSingular ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think it's possible to commit a transaction and still return a 4xx
/ 5xx
error code via GUC: https://postgrest.org/en/latest/api.html#setting-response-status-code
This means, that you can not tell from the error code alone whether the transaction was comitted or rolled back.
Is it possible to refactor to avoid this change?
I glanced over most files, but not Many of my comments are about import statements - and I guess half of those I know we should get this PR merged rather quickly, because it touches so much code - it will be a pain to keep this open. |
88142cf
to
8ed8668
Compare
Yes, but no need to hurry - I'll rebase as needed |
ef923ee
to
c93324e
Compare
a6ba878
to
1e6c272
Compare
@wolfgangwalther Suggest that we defer a few changes to a separate PR, please see the comments above. All others should be reflected. |
@wolfgangwalther The coverage failure happens because we're no longer using the record fields in App.hs directly, instead we always unpack the records. Should probably be one of the coverage exceptions once we have full coverage. |
src/PostgREST/App.hs
Outdated
_ -> | ||
SQL.Write | ||
|
||
data CreateResult = CreateResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does create mean here? I see it's used in all kind of modifying queries (delete, update, ...).
I think this needs a better name - and the fields a better prefix, too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, was difficult to follow. Added a comment and renamed.
This type and the writeQuery
function are some glue to adapt the DbRequestBuilder
and Statements
APIs to something that is easier to handle. They should move to one of those modules when we sort out the imports. I didn't want to touch those modules in this PR too, it's unwieldy enough as it is :-)
I now reviewed the remaining parts of
I did that and there is no difference in minimum latency with the vegeta test at all. That's only for a plain |
The initial increase in code coverage was not real, but just an effect of the refactor having a lot more lines of code initially, which increased the percentage. However I checked the open coverage TODOs in #1719 for App.hs. Because of the changes to error handling, 3 of the 6 are done with this PR.
Absolutely, I agree. Others have that problem, too - let's hope somebody will do something about it: https://gitlab.haskell.org/ghc/ghc/-/issues/17834 |
207e91c
to
c32303f
Compare
Many thanks for your thorough review and helpful comments @wolfgangwalther ! I added a few commits to address your notes. |
c32303f
to
dfa5345
Compare
The freebsd builds fails with
No idea what change would have caused that. Could that be some caching issue @steve-chavez ? |
@monacoremo I'm also getting the same error. It could be related to caching because we cache freebsd packages in: Lines 7 to 13 in 0ddd676
I'll try removing this cache on #1760. (Seems related to https://reviews.freebsd.org/D25117) |
Thank you @wolfgangwalther and @steve-chavez reviewing and helping to get this PR done! I'll merge this now so that we can push ahead on other topics, even though we'll get a CI failure for the unrelated FreeBSD issue. |
Bummer, this didn't work: It must be something else. Perhaps we can try busting the cache with I'll refrain from doing this on #1760 though, it already got a bit long. |
* Use ExceptT to avoid 'staircasing' case analysis in App.hs * Split large function in App.hs into individual handler functions * Adapt API of Auth.hs, OpenApi.hs etc. to simplify the use of those modules in App.hs * Split optional rollback functionality into Middleware * Unify SimpleError and ApiRequestError into one Error type, so it can be used across modules
I struggled to follow what was happening in
App.hs
, so I tried to simplify the structure by decomposing theapp
function, which has grown to become quite huge over time.The result is more verbose, but IMO also easier to follow and maintain. I kept the refactor purely to
App.hs
to keep the diff simpler, but there are some obvious simplifications in other modules that result from this.I'm not a fan of the right-padding on imports and in type declarations, as they are a pain to maintain and lead to unclean diffs. Maybe that's something we can reconsider in our stylishhaskell settings?