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

refactor: Split Request, Query and Response interfaces from App #1831

Closed
wants to merge 1 commit into from

Conversation

monacoremo
Copy link
Member

Work in progress: Create a small API for all the request parsing we do - this will simplify App, enable more unit testing and allow us to simplify/refactor Request and it's submodules more easily.

@monacoremo monacoremo force-pushed the refactorrequest branch 2 times, most recently from 11c37bd to cde8645 Compare April 26, 2021 16:38
@monacoremo
Copy link
Member Author

@wolfgangwalther it's becoming obvious that #1814 (comment) was a lie...

This splits up pieces from App into Request, Query and Response, as we discussed back in #1793. There will still be many unwanted interlinks between these modules, but it's a first step. The split of Query follows your recommendation, much better than the earlier attempt. WDYT?

@wolfgangwalther
Copy link
Member

@wolfgangwalther it's becoming obvious that #1814 (comment) was a lie...

All good. I was a bit bored by #1830 already.

This splits up pieces from App into Request, Query and Response, as we discussed back in #1793. There will still be many unwanted interlinks between these modules, but it's a first step. The split of Query follows your recommendation, much better than the earlier attempt. WDYT?

Hm. I had hoped we could get to a point where all IO handling was in App - and Request, Query and Response were all pure. The execution of the query would then be in App - while Query would construct and return it. Right now, it's a bit too much of App that is moved into Query, if I understood correctly.

Do you think that's possible / would make things easier? My hope is that unit testing of query generation would be easier to do.


Also what strikes me as odd is the fact, that App pattern matches for each different request type - and then we have query and response handlers for each request type individually. I guess this is mostly a "we're not there, yet" - do you think it will be possible to have the IO part in App made generic? I.e. pass Request into createQuery (or whatever the name would be) - then pattern match there. Then pass Request and QueryResult in createResponse (or whatever ...) etc.


I really like those interfaces:

updateResponse :: ApiRequest -> WriteQueryResult -> Wai.Response

singleUpsertResponse :: ApiRequest -> WriteQueryResult -> Wai.Response

deleteResponse :: ApiRequest -> WriteQueryResult -> Wai.Response

Those are still a bit heavy:

readResponse :: Bool -> QualifiedIdentifier -> AppConfig -> ApiRequest -> Statements.ResultsWithCount -> Maybe Int64 -> [GucHeader] -> Maybe HTTP.Status -> Wai.Response

createResponse :: QualifiedIdentifier -> DbStructure -> ApiRequest -> WriteQueryResult -> Wai.Response

invokeResponse :: InvokeMethod -> ApiRequest -> Statements.ProcResults -> [GucHeader] -> Maybe HTTP.Status -> Wai.Response

This looks like we need to either extend ApiRequest with a bit more things - or wrap it in a Request together with other stuff.

@monacoremo
Copy link
Member Author

Also what strikes me as odd is the fact, that App pattern matches for each different request type - and then we have query and response handlers for each request type individually.

Thought about this also, and how it would be nice to chain one set of three functions for all cases. I think this way seems a bit less elegant, but makes more impossible states impossible: It's obvious that no update query will ever be rendered into a singleUpsert response just by looking at App. Let's see, but it might be the stable outcome.

This looks like we need to either extend ApiRequest with a bit more things - or wrap it in a Request together with other stuff

Yes, definitely - the signatures of both the Query and Response functions are a mess. I'm trying to see what the overall pattern is, it's not the RequestContext we previously had. I might completely wrap ApiRequest and only expose Request to get there...

@wolfgangwalther
Copy link
Member

Hm. I had hoped we could get to a point where all IO handling was in App - and Request, Query and Response were all pure. The execution of the query would then be in App - while Query would construct and return it. Right now, it's a bit too much of App that is moved into Query, if I understood correctly.

Do you think that's possible / would make things easier? My hope is that unit testing of query generation would be easier to do.

Hm. Everything from handleRequest on is already pure, right? I'm still confused by Monads in general... and runDbHandler was hidden in the diff. Aha!

@monacoremo
Copy link
Member Author

Hm. I had hoped we could get to a point where all IO handling was in App - and Request, Query and Response were all pure

We are pretty close actually!

  • Request returns Either
  • Query returns a DbHandler (more below)
  • Response returns a pure Wai.Response

A DbHandler is a Hasql.Transaction whose execution (and theoretically creation, need to check whether we have such cases left) can fail with an Error.

We should be able to split this out further within Query, but as a first step this makes sense I think.

@wolfgangwalther
Copy link
Member

A DbHandler is a Hasql.Transaction whose execution (and theoretically creation, need to check whether we have such cases left) can fail with an Error.

DbHandler seems to be defined twice. Once in App.hs and once in Query.hs. Is that on purpose?

@monacoremo monacoremo force-pushed the refactorrequest branch 2 times, most recently from 2b6ec9f to a22d914 Compare April 29, 2021 14:42
@monacoremo monacoremo changed the title refactor: Carve out simple interface for request parsing refactor: Split Request, Query and Response interfaces from App Apr 29, 2021
@monacoremo
Copy link
Member Author

DbHandler seems to be defined twice. Once in App.hs and once in Query.hs. Is that on purpose?

Was not on purpose! Fixed. I think that the whole runDbHandler stuff will move to Query later on, but I'm leaving it as is fow now.

@monacoremo monacoremo marked this pull request as ready for review April 29, 2021 17:17
@monacoremo
Copy link
Member Author

I think we should make a cut here! The dispatch in App should be pretty stable, as are the interfaces of Request, Query and Response. Each of those modules and their submodules still have significant refactoring potential. This will be easier to do now that they have clear interfaces.

@monacoremo monacoremo mentioned this pull request Apr 29, 2021
27 tasks
src/PostgREST/Auth.hs Outdated Show resolved Hide resolved
Copy link
Member

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Reviewed "everything" except Query.hs, Request.hs and Response.hs so far. So... basically nothing, yet. :D

src/PostgREST/App.hs Outdated Show resolved Hide resolved
src/PostgREST/App.hs Show resolved Hide resolved
src/PostgREST/Auth.hs Outdated Show resolved Hide resolved
JWT.verifyClaimsAt validation secret time =<< JWT.decodeCompact payload
liftEither . mapLeft jwtClaimsError $ claimsMap configJwtRoleClaimKey <$> eitherClaims
AppConfig -> Request -> UTCTime -> ExceptT Error m JWTClaims
jwtClaims AppConfig{..} request time =
Copy link
Member

Choose a reason for hiding this comment

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

We need some kind of naming convention for PostgREST.Requests and Wai.Requests. In App.hs it was request and req. Now request is the other one.

I wonder whether we should maybe not use Request and Response at all, given that those clash with wai like that.

What about RestRequest and RestResponse?

And then consistently use request for Wai.Request and restRequest for PostgREST.RestRequest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I think it would be cleanest to use PostgREST.Requestand PostgREST.Response if possible in our codebase. The most impacted module is PostgREST.App, let me see if I can make it clearer there.

Copy link
Member

Choose a reason for hiding this comment

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

request and waiRequest for names then?

src/PostgREST/Query/Statements.hs Outdated Show resolved Hide resolved
Copy link
Member

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

This second review is mostly about the interfaces between App, Request, Query and Response. I haven't really looked into the 3 modules in detail, yet - that's something I will do in a third round, to make sure that no unrelated changes sneaked in.

From what I can tell right now, I really like how things have been split up and where the different parts ended up.

However, I don't like the interface between those modules, too much, yet. Specifically:

  • App currently needs to know about a few implementation details, namely:
    • The explicit pass-through of xxxRequestInfo. This type is purely internal actually and just a workaround to the partiality problem for constructors with different record fields.
    • The mere fact that each "path" (read, create, ...) is separated in each module. Whether some of those paths are handled by the same function internally or not, should be up to the module. It would be better to have one entrypoint per module - just like Request.parse.
  • On the other side some part of the interface are hidden, because they are passed on through the xxxRequestInfo and xxxQueryResult types: config, version, structure in both cases, and the request in QueryResult. The response function's interface should really be something like respond :: Context -> Request -> Result -> Wai.Response. Right now each step kind of takes their input and wraps it in the output. Strange!

I know that this interface resulted from "making impossible states impossible", i.e. stronger typing - at compile time. That's an absolute positive.

But, I think we can get both. Generalize App, make the interfaces really nice - and have strong typing. Here's how:

(Note: this includes suggestions in some of the other comments below, might need to read those first - mainly about naming etc.)

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE KindSignatures #-}

data Context = Context
  { ctxConfig      :: AppConfig
  , ctxPgVersion   :: PgVersion
  , ctxDbStructure :: DbStructure
  }

data RequestType
  = Read
  | Create
  | Update
  | Upsert
  | Delete
  | Invoke
  | Info
  | OpenApi

data Request (t :: RequestType) where
  ReadRequest :: ApiRequest -> ReadTree -> Request 'Read
  CreateRequest :: ApiRequest -> ReadTree -> MutateQuery -> Request Create
  UpdateRequest :: ApiRequest -> ReadTree -> MutateQuery -> Request Update
  UpsertRequest :: ApiRequest -> ReadTree -> MutateQuery -> Request Upsert
  DeleteRequest :: ApiRequest -> ReadTree -> MutateQuery -> Request Delete
  InvokeRequest :: ApiRequest -> ReadTree -> Request Invoke
  InfoRequest :: ApiRequest -> Request Info
  OpenApiRequest :: ApiRequest -> Request OpenApi

data Result (t :: RequestType) = Result
  { resBody       :: ByteString
  , resGucHeaders :: [GucHeader]
  , resGucStatus  :: Maybe HTTP.Status
  , resQueryTotal :: Int64
  , resTableTotal :: Maybe Int64
}

-- public interfaces (towards App)
Request.parse :: Context -> Wai.Request -> LBS.ByteString -> Either Error (Request t)
Query.build :: Context -> Request t -> DbHandler (Result t)
Response.send :: Context -> Request t -> DbHandler (Result t) -> Wai.Response

-- examples for internal interfaces
readQuery :: Context -> Request Read -> DbHandler (Result Read)
createQuery :: Context -> Request Create -> DbHandler (Result Create)
-- ...
readResponse :: Context -> Request Read -> DbHandler (Result Read) -> Wai.Response
createResponse :: Context -> Request Create -> DbHandler (Result Create) -> Wai.Response
-- ...

This would even be more type-safe than the current interface, where you could pass a MutateQueryResult from e.g. createQuery into updateResponse or so...

We can still easily see from the public interfaces, that it's not possible to mix different request and response types (Request t -> Result t). App is completely generic and just puts the different parts together. None of the 3 modules is limited regarding internal refactoring, because they each have 1 entrypoint only.

Comment on lines +123 to +128
waiBody <- lift $ Wai.strictRequestBody waiRequest
request <- liftEither $ Request.parse conf pgVer dbStructure waiRequest waiBody
Copy link
Member

Choose a reason for hiding this comment

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

I think body was fine. waiBody makes me second-guess - is there something "in" there, that I am missing? It's just a lazy bytestring, isn't it?

Comment on lines +52 to +82
data ReadRequestInfo = ReadRequestInfo
{ rrConfig :: AppConfig
, rrPgVersion :: PgVersion
, rrDbStructure :: DbStructure
, rrApiRequest :: ApiRequest
, rrHeadersOnly :: Bool
, rrIdentifier :: QualifiedIdentifier
, rrReadRequest :: Types.ReadRequest
, rrBinaryField :: BinaryField
}

data MutateRequestInfo = MutateRequestInfo
{ mrConfig :: AppConfig
, mrPgVersion :: PgVersion
, mrDbStructure :: DbStructure
, mrApiRequest :: ApiRequest
, mrIdentifier :: QualifiedIdentifier
, mrReadRequest :: Types.ReadRequest
, mrMutateRequest :: Types.MutateRequest
}

data InvokeRequestInfo = InvokeRequestInfo
{ irConfig :: AppConfig
, irPgVersion :: PgVersion
, irDbStructure :: DbStructure
, irApiRequest :: ApiRequest
, irInvokeMethod :: InvokeMethod
, irProc :: ProcDescription
, irReadRequest :: Types.ReadRequest
, irBinaryField :: BinaryField
}
Copy link
Member

Choose a reason for hiding this comment

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

(Might be irrelevant when generalizing the interface as mentioned in the opening comment)

I think it would make the code more readable to use DuplicateRecordFields and have those different types use the same field prefix req. All the names that match are exactly the same. rrConfig, mrConfig, irConfig - all reqConfig, etc.

Also, the two letter prefixes are a tad too short, I think. It's almost as iXX with ApiRequest. Although, that's a bit tougher, because I don't see any connection at all. req is immediately clear.

Comment on lines +42 to +50
data Request
= ReadRequest ReadRequestInfo
| CreateRequest MutateRequestInfo
| UpdateRequest MutateRequestInfo
| SingleUpsertRequest MutateRequestInfo
| DeleteRequest MutateRequestInfo
| InfoRequest DbStructure ApiRequest QualifiedIdentifier
| InvokeRequest InvokeRequestInfo
| OpenApiRequest AppConfig DbStructure ApiRequest Bool Schema
Copy link
Member

Choose a reason for hiding this comment

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

(Might be irrelevant when generalizing the interface as mentioned in the opening comment)

Info seems off. It suggests some additional, non-essential information. But this is really the meat of everything.

I suggest to rename the XXXRequestInfo to XXXRequestPayload, XXXRequestContext, XXXRequestFields or something like that. I think I like "fields" most.

The additional type added here is really just to avoid partial application of accessors, but is otherwise not really exposed - at least I don't consider it a valuable part of the interface. If this wasn't a problem we would most likely add them as record fields directly on the Request constructors, right? I think XXXRequestFields matches that intent best.

Comment on lines +142 to +156
ReadRequest readRequestInfo ->
Response.readResponse <$> Query.readQuery readRequestInfo
CreateRequest mutateRequestInfo ->
Response.createResponse <$> Query.createQuery mutateRequestInfo
UpdateRequest mutateRequestInfo ->
Response.updateResponse <$> Query.updateQuery mutateRequestInfo
SingleUpsertRequest mutateRequestInfo ->
Response.singleUpsertResponse <$> Query.singleUpsertQuery mutateRequestInfo
DeleteRequest mutateRequestInfo ->
Response.deleteResponse <$> Query.deleteQuery mutateRequestInfo
Copy link
Member

Choose a reason for hiding this comment

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

(Might be irrelevant when generalizing the interface as mentioned in the opening comment)

I liked the "same name for all info types" from before better. For me, the difference between those is really not important here. This is just about "passing through" the fields that should actually be part of Request directly (see other comment).

Comment on lines +58 to +83
data ReadQueryResult = ReadQueryResult
{ rqRequest :: ReadRequestInfo
, rqQueryTotal :: Int64
, rqBody :: BS8.ByteString
, rqTableTotal :: Maybe Int64
, rqGucHeaders :: [GucHeader]
, rqGucStatus :: Maybe HTTP.Status
}

data MutateQueryResult = MutateQueryResult
{ resRequest :: MutateRequestInfo
, resQueryTotal :: Int64
, resFields :: [ByteString]
, resBody :: ByteString
, resGucStatus :: Maybe HTTP.Status
, resGucHeaders :: [GucHeader]
}

data InvokeQueryResult = InvokeQueryResult
{ iqRequest :: InvokeRequestInfo
, iqTableTotal :: Maybe Int64
, iqQueryTotal :: Int64
, iqBody :: BS8.ByteString
, iqGucHeaders :: [GucHeader]
, iqGucStatus :: Maybe HTTP.Status
}
Copy link
Member

Choose a reason for hiding this comment

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

(Might be irrelevant when generalizing the interface as mentioned in the opening comment)

Same reasoning as with the request info types: Let's use the same prefix and DuplicateRecordFields here.

MutateQueryResult already has the correct prefix!


Actually, on second thought: Do we really need 3 types here? They are all the same - almost. resTableTotal could easily be added to MutateQueryResult - it's Maybe anyway. And resRequest... should just not be in here at all, I think.

It would be much nicer to pass request and result separately to the response functions.

Comment on lines +63 to +70
data MutateRequestInfo = MutateRequestInfo
{ mrConfig :: AppConfig
, mrPgVersion :: PgVersion
, mrDbStructure :: DbStructure
, mrApiRequest :: ApiRequest
, mrIdentifier :: QualifiedIdentifier
, mrReadRequest :: Types.ReadRequest
, mrMutateRequest :: Types.MutateRequest
Copy link
Member

Choose a reason for hiding this comment

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

I am not really happy with the ambiguity of ReadRequest, ReadRequestXXX and MutateRequestXXX on one side and Types.ReadRequest and Types.MutateRequest on the other.

I think we should rename the two Types.XXX ones. Types.MutateRequest is just an alias for MutateQuery - let's use this. And Types.ReadRequest could very well be type ReadTree = Tree ReadNode.

Comment on lines +57 to +58
, rrHeadersOnly :: Bool
, rrIdentifier :: QualifiedIdentifier
Copy link
Member

Choose a reason for hiding this comment

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

Those two are already part of ApiRequest, right?

There are some in the other Info types, too. Removing those, and using everything from ApiRequest, would streamline the interface a bit more.

src/PostgREST/Request.hs Show resolved Hide resolved
@steve-chavez
Copy link
Member

I like the idea of a single entrypoint per module but I'm having trouble with getting GADTs to compile.

Assuming the following reduced example, how would the parse function work?

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE KindSignatures #-}

data RequestType
  = Create
  | Update

data Request (t :: RequestType) where
  CreateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request Create
  UpdateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request Update

-- single entrypoint for Request.hs
parse :: ApiRequest -> ReadRequest -> MutateRequest -> Request t
parse apiReq readReq mutateReq =
  case iAction apiReq of
    ActionMutate MutationUpdate -> UpdateRequest apiReq readReq mutateReq
    _                           -> CreateRequest apiReq readReq mutateReq

This produces

$ postgrest-build

src/PostgREST/App.hs:102:36: error:
    • Couldn't match type ‘t’ with ‘'Update’
      Expected: Request t
        Actual: Request 'Update
      ‘t’ is a rigid type variable bound by
        the type signature for:
          parse :: forall (t :: RequestType).
                   ApiRequest -> ReadRequest -> MutateRequest -> Request t
        at src/PostgREST/App.hs:99:1-64
    • In the expression: UpdateRequest apiReq readReq mutateReq
      In a case alternative:
          ActionMutate MutationUpdate
            -> UpdateRequest apiReq readReq mutateReq
      In the expression:
        case iAction apiReq of
          ActionMutate MutationUpdate
            -> UpdateRequest apiReq readReq mutateReq
          _ -> CreateRequest apiReq readReq mutateReq
    • Relevant bindings include
        parse :: ApiRequest -> ReadRequest -> MutateRequest -> Request t
          (bound at src/PostgREST/App.hs:100:1)
    |
102 |     ActionMutate MutationUpdate -> UpdateRequest apiReq readReq mutateReq

If the type is changed to

data Request (t :: RequestType) where
  CreateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request t
  UpdateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request t

It does compile but that would defeat the purpose of GADTs.

@wolfgangwalther
Copy link
Member

I like the idea of a single entrypoint per module but I'm having trouble with getting GADTs to compile.

It took me a few attempts to understand GADTs and DataKinds a little bit.. and I'm still not sure whether I fully grasp them. I will look at this in the next few days and answer a bit more detailed.

@wolfgangwalther
Copy link
Member

Assuming the following reduced example, how would the parse function work?
[...]

src/PostgREST/App.hs:102:36: error:
    • Couldn't match type ‘t’ with ‘'Update’
      Expected: Request t
        Actual: Request 'Update
      ‘t’ is a rigid type variable bound by
        the type signature for:
          parse :: forall (t :: RequestType).
                   ApiRequest -> ReadRequest -> MutateRequest -> Request t
        at src/PostgREST/App.hs:99:1-64

I had exactly the same problem just a few days ago. Here's my current take on it.

Polymorphic functions in haskell work much like they do in postgres: They need a polymorphic input type - from which the return type can be derived. This is not possible in the parse function, because it has a type variable on the return only:

parse :: ApiRequest -> ReadRequest -> MutateRequest -> Request t

No way for haskell to tell which type it should actually use as the return type at compile time. So we need a parametrized input type as well. The way I think about this is, that I need to be able to draw some kind path between both ends of everything that needs to be polymorphic. I need to tie the concrete types together, basically. I can tie one end to the Request Create or Request Update - but the other end is loose right now.

The following won't exactly work, but you get the idea:

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE KindSignatures #-}

data RequestType
  = Create
  | Update

data Mutation (t :: RequestType) where
  MutationCreate :: Mutation Create
  MutationUpdate :: Mutation Update

data Action (t :: RequestType) where
  ActionMutate :: Action (Mutation t)

data ApiRequest (t :: RequestType) where
  ApiRequest :: { iAction :: Action t } -> ApiRequest t

data Request (t :: RequestType) where
  CreateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request Create
  UpdateRequest :: ApiRequest -> ReadRequest -> MutateRequest -> Request Update

-- single entrypoint for Request.hs
parse :: ApiRequest t -> ReadRequest -> MutateRequest -> Request t
parse apiReq readReq mutateReq =
  case iAction apiReq of
    ActionMutate MutationUpdate -> UpdateRequest apiReq readReq mutateReq
    _                           -> CreateRequest apiReq readReq mutateReq

Now, the other end is tied to the Mutation type - and you can draw a path through all the parametrized types and functions from Mutation Create to Request Create.

Now, you will still have a similar challenge for the userApiRequest function (or whatever the function was called that creates a new ApiRequest and returns it to App. This will have to return different types of ApiRequests, but you can't parametrize the input - after all the decision which kind of request it is, is made inside this function.

For this to work, you need a container type for the ApiRequest, which itself is not parametrized. It kind of hides the parameter:

data AnyApiRequest = forall any. AnyApiRequest (ApiRequest any)

userApiRequest :: whatever -> AnyApiRequest

This function is not polymorphic anymore, so this works.

You can then extract the actual ApiRequest from the AnyApiRequest in App.hs and pass it on to parse etc.

Let me know whether that makes any sense or where I should try to put it in different words.

@steve-chavez
Copy link
Member

@wolfgangwalther Thanks for the explanation, It's clear now!

I've linked your comment on #1804. I think that's better left for another PR since ApiRequest needs other changes too(like #1804 (comment)).

I'll close this one since Query, Response and Plan(#2497, which replaces the Request module here) have being added in other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants