[dev] Refactoring roadmap

[monacoremo]

With #1793, we've started the process of streamlining the codebase. This is a purely internal concern that will yield no additional features. It will, however:

• ease maintenance
• help with the onboarding of new contributors
• enable quicker implementation of new features
• further increase confidence on correctness

Next steps include (in no particular order and to be completed - please feel free to edit)

• Split CLI and Config modules (Carve CLI out of Config #1806)
• Streamline main/Main.hs, minimizing code under CPP conditional compilation (refactor: carve out modules from Main.hs #1814)
• Split steps in request processing, e.g. along the pipeline: (finished on refactor: remove SchemaCache from ApiRequest #2695)
  • Request: purely parse request, at most taking config into account. `parseRequest :: Wai.Request -> Either RequestError Request
  • Plan: Use db structure to validate and plan the request, including embeds. plan :: DbStructure -> Request -> Either PlanError QueryPlan
  • Query: Run the planned query. `query :: QueryPlan -> Transaction (Either QueryError QueryResult)
  • Response: Create a HTTP response out of the result. reponse :: Plan -> QueryResult -> Wai.Response
• Single entrypoint for Plan/Query/Respones based on GADTs refactor: Split Request, Query and Response interfaces from App #1831 (review)
• Encapsulate the Request modules behind one API (refactor: Split Request, Query and Response interfaces from App #1831)
• Encapsulate the Query modules behind on API (refactor: Split Request, Query and Response interfaces from App #1831, refactor: add Query.hs module #2488)
• Carve-out Response concerns from App, i.a. (refactor: Split Request, Query and Response interfaces from App #1831, refactor: add Response.hs module #2484)
• Split Error type along Request, Query and Response
• Stop using GHC.Show instances for string conversion (Refactor: Remove (mis)uses of GHC.Show instances #2016)
• Stop using partial functions (ones from Protolude.Conv, unsafeHead etc.)
  • Protolude.Conv.toS (Refactor: Remove uses of Protolude.Conv.toS #2017)
  • decodeUtf8 et. al
  • unsafeHead
  • ...
• Improve string handling in Query generation for additional safety and performance
• Use doctest for unit testing within modules where needed (Add doctests #2019)
• Add an ARCHITECTURE.md or similar to guide newcomers to the codebase, explaining how the major modules fit together
• Add io test for parsing the example config file Carve CLI out of Config #1806 (comment)
• Clean up how Config is loaded from different sources and parsed
• Use AppState as a monad (tbd)
• Make readPGRSTEnv redundant in Main, e.g. by making all opts optional and failing in the parser
• Sort out loading of DB schema and settings on startup
• Make imports and import aliases consistent, check them in CI using hsie (Refactor: Make import aliases consistent #2008)
• Add documentation to modules and exposed functions/types
• Represent the AppState as a sum type
• ...

Other:

• Add a postgrest-install-git-hooks script

[steve-chavez]

Putting another pending refactor here.

We had an issue where Content-Type was being considered for GET - invalid since it has no body. This happened mostly because a typing deficiency(still present). More details at #2168 (comment).

[steve-chavez]

unsafeHead went away on #2254

[steve-chavez]

Request: purely parse request, at most taking config into account. `parseRequest :: Wai.Request -> Either RequestError Request

Plan: Use db structure to validate and plan the request, including embeds. plan :: DbStructure -> Request -> Either PlanError QueryPlan

To do the above, ApiRequest should remove its dependency on DbStructure - which is needed by the variadic logic introduced on #1603.

postgrest/src/PostgREST/Request/ApiRequest.hs

Line 135 in 6398dd2

if not $ pdHasVariadic proc then -- if proc has no variadic param, save steps and directly convert to json

After that findProc and others can be moved to Plan.hs and ApiRequest will be much cleaner.

I tried to do this on #2496 but it turned out difficult bc I'm not familiar with the variadic logic. @wolfgangwalther Would be great if you could help with that.

---

Ideally the proc logic shouldn't be where the body processing is done. Like on MTUrlEncoded here

postgrest/src/PostgREST/Request/ApiRequest.hs

Line 343 in 6398dd2

(MTUrlEncoded, ActionInvoke InvPost) -> targetToJsonRpcParams (rightToMaybe target) $ (T.decodeUtf8 *** T.decodeUtf8) <$> parseSimpleQuery (LBS.toStrict reqBody)

That logic should happen after, on the Plan module.

[wolfgangwalther]

The challenge is, that ApiRequest does pass on URL parameters as iPayload, i.e. payload is either the request body or a json produced from the params. ApiRequest needs to split this up and have the params as just a [(Text, Text)] list. Then, Plan can do the list -> map later, taking the ProcDescription into account.

[steve-chavez]

I've been thinking about how we transform requests, and seeing the The Internals of PostgreSQL query processing chapter here:

1. Parser
The parser generates a parse tree from an SQL statement in plain text.

2. Analyzer/Analyser
The analyzer/analyser carries out a semantic analysis of a parse tree and generates a query tree.

3. Rewriter
The rewriter transforms a query tree using the rules stored in the rule system if such rules exist.

4. Planner
The planner generates the plan tree that can most effectively be executed from the query tree.

5. Executor
The executor executes the query via accessing the tables and indexes in the order that was created by the plan tree.

It seems:

• Our ApiRequest is the Parser
• Analyzer is really what we call Plan now.
• The Rewriter concept could be used to further split our Plan module, which is getting big. For example computed relationships could be thought of as a "rewrite". Data Representations #2523 seems like a rewrite too.
• We don't have a Planner per se. This is a further optimization that perhaps could be used to reduce generated prepared statements, e.g. select=id,name could generate the same query as select=name,id. Not sure about this.
  • Maybe we could have sort of a "prepared request" akin to pg "prepared statement".
• Executor doesn't make sense for us.

[steve-chavez]

Continuing the above idea. Our App.hs module can be equated to the TrafficCop in PostgreSQL https://wiki.postgresql.org/wiki/Backend_flowchart#Main_Query_Flow

These ideas can be used to structure our modules and to improve the ARCHITECTURE.md.

[steve-chavez]

There are still some pending refactors but overall the structure now is in line with the original comment. Will unpin this issue as it's not urgent anymore. We can now grow the codebase in a more maintainable manner.

[dbaynard]

Congratulations — great job!

• help with the onboarding of new contributors

As a (current) non-contributer I'm willing to help test this.