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

[Epic] Decouple FE from the details of MBQL; write it in CLJC to unify logic with BE #28034

Closed
7 of 10 tasks
bshepherdson opened this issue Feb 2, 2023 · 2 comments
Closed
7 of 10 tasks
Assignees
Labels
.Epic Feature Implementation or Project .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib .Team/QueryProcessor :hammer_and_wrench:

Comments

@bshepherdson
Copy link
Contributor

bshepherdson commented Feb 2, 2023

Background

Many parts of the Query Builder have code that cares about the gory details of MBQL queries. This is a major source of friction and complexity in the QB code, and a major source of small corner case bugs. Bugs get introduced when the FE's and BE's understanding of a query diverge, and when corner cases are missed in testing when a new feature is added (eg. forgetting to check multi-stage queries).

As an example of the details leaking out, some QB views check where a question is (a) based on a regular source-table, ie. a table in the warehouse; (b) based on another saved question, possibly a model; or (c) is a multi-stage nested query whose innermost query might be (a) or (b).

Most of the code in frontend/src/metabase-lib deals with Metabase domain entities (Cards and Dashboards, Queries and Filters, Dimensions and Actions) and contains a lot of business logic around building, updating and querying them.

Goals

  • Idiomatic: both Clojure and JS/TS code should be comfortable working with these objects.
  • Immutable: this is standard in Clojure, and these domain entities are immutable in TS currently (up to some caching). We will preserve this style.
  • Opaque: Nothing outside metabase-lib should know anything about the inner structure. All access is through library functions and the returned values are treated as opaque blobs.
  • TypeScript: All functions, objects and methods in this library should have TS types. (Either hand-written or possibly generated from Malli schemas.)

Guidance

This code is likely to undergo several rounds of further refactoring in the future, so let's set ourselves up for success by making that easy to achieve.

Follow Clojure conventions for organizing namespaces; we are under no obligation to match the original folder structure. Also prefer putting code under src/, not shared/ - shared/ is considered deprecated.

Eventual integration

The long-term plan is static TS functions which are mostly one-line wrappers for CLJC functions. The objects being passed around (Card, Table, etc.) are CLJS maps treated as opaque by the FE code. This can be nicely enforced in TS with a bit of sleight of hand that creates distinct, "unforgeable", opaque types. (That is, TS can't see inside an opaque Query, and if you use that opaque Query in place of a Card, you get a clear error.)

We can enforce with the linter that only the frontend/src/metabase-lib code is allowed to import from cljs/metabase.lib.*. This will help future refactorings by giving us some indirection, and only one real call site to change.

Transitional TS Classes

While we're making incremental ports, we'll be moving logic from the Question, StructuredQuery etc. classes into CLJC functions. For now it's easy to make the methods simply wrap the CLJC functions. Eventually we'll refactor the call sites to just use the static functions in metabase-lib/ and these classes can disappear.

On Memoization

Some of the TS classes use a memoization helper with a pattern like this:

class StructuredQueryInner extends BlahBlah {
  // real implementation here...
}

export class StructuredQuery extends memoizeClass<StructuredQueryInner>(
  "memoizedMethod",
  "namesGoHere",
)(StructuredQueryInner) {}

That's actually transparent to these ports, so no worries. (Unless you're removing one of the memoized methods. In that case just drop it from the list.)

On Testing

Write CLJC tests for the logic on the Clojure side.

Keep the JS/TS tests for the TS classes for things that still exist in TS. If you're deleting a helper function with TS tests, port the test cases into CLJC as well and delete the TS tests.

On CLJS and CLJC

Everything should be CLJC by default. We need a strong reason to write any separate CLJ and CLJS code here. The eventual goal is to have FE and BE both using the same logic for wrangling the Metabase domain entities like MBQL queries.

On Subclasses

A few of the entities ({Native,Structured}Query, *Dimension) have nontrivial class hierarchies to them. We can preserve that during the transition, but eventually the division should disappear.

Aim to write the CLJC functions as able to handle all the variations transparently, so the FE can call them without having to know or care about the differences. (Of course there are differences that matter - native queries and MBQL queries are handled differently in a few key places, like the native query editor! But lots of other places don't need to care about even that difference.)

Milestones

Milestone 0: Proof of concept

Milestone 0, part 2: Rebuild to opaque CLJS blob

Milestone 1: Port LIMIT

Milestones 2-4: Port order-by, aggregations, and filters (probably in that order?)

Milestones 5+: Keep chasing callers of legacy metabase-lib/ functionality and porting them

eg. many components pass around Questions, but once we make MLv2 Cards available in the Redux store, they should all be ported (in pieces) to use MLv2.

There's a fair bit of this, but it's hard to map out and most of it can happen in any order.

@bshepherdson bshepherdson added the .Epic Feature Implementation or Project label Feb 2, 2023
@bshepherdson bshepherdson self-assigned this Feb 2, 2023
@bshepherdson bshepherdson added the .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib label Feb 13, 2023
@bshepherdson
Copy link
Contributor Author

bshepherdson commented Feb 14, 2023

PSA: There's a distressing mix of identifier spelling in these underlying types. Broadly the JS objects are using snake_case, but there are occasional camelCase ones and even a few "lisp-case" with explicit strings!

The default return format for outgoing objects is snake_case strings. However, I'm adding support to the converters to walk the schema looking for {:js-prop "camelCase"} properties to override. Once that lands it should work transparently if we mark the fields in the schemas properly.

@bshepherdson bshepherdson changed the title [Epic] Port metabase-lib to CLJC; share code with BE [Epic] Decouple FE from the details of MBQL; use CLJC to unify logic with BE Mar 14, 2023
@darksciencebase darksciencebase added the .Team/QueryProcessor :hammer_and_wrench: label Mar 22, 2023
@bshepherdson bshepherdson changed the title [Epic] Decouple FE from the details of MBQL; use CLJC to unify logic with BE [Epic] Decouple FE from the details of MBQL; write it in CLJC to unify logic with BE Mar 27, 2023
@darksciencebase
Copy link
Contributor

Closing in favor of the sub-epics and sub-tasks of #28689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Epic Feature Implementation or Project .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Development

No branches or pull requests

2 participants