WIP: Refactor db services and query building #218
Conversation
To improve the maintainability, readabiity and testability of the API codebase some major refactors were implemented. The BaseSupabaseService is executes different strategies and applies where filtering and sorting. Query Strategies and Factory were created for clear declaration of querying strategies Tests were updated and basic tests for the services created. Including getting the graphql tests running with vitest
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||
|
|
||
| export function withPagination<TItem extends ClassType>(TItemClass: TItem) { | ||
| export function withPagination<TItem extends ClassType>(TItemClass: TItem): B { |
There was a problem hiding this comment.
I'm confused, where does B come from?
| @@ -25,5 +27,7 @@ export type WhereOptions<T extends object> = { | |||
| | BasicContractWhereInput | |||
| | BasicFractionWhereInput | |||
| | BasicSignatureRequestWhereInput | |||
| | BasicAttestationWhereInput | |||
There was a problem hiding this comment.
I find it difficult to know where in our code base we need to add which classes etc. Can we perhaps generate that somehow? If there was one file, that consolidated all the entities and their where-inputs and whatnot and we generate this type from there it would be a much better DX.
| import { GetAttestationsArgs } from "../args/attestationArgs.js"; | ||
| import { GetAttestationSchemasArgs } from "../args/attestationSchemaArgs.js"; |
There was a problem hiding this comment.
Could be cool to have an /args/index.ts that exports all args and we do
import QueryArgs from "../args";
in this file?
| @@ -2,32 +2,40 @@ import { expressionBuilder, Kysely, SqlBool } from "kysely"; | |||
| import { BaseArgs } from "../graphql/schemas/args/baseArgs.js"; | |||
| import { SortOrder } from "../graphql/schemas/enums/sortEnums.js"; | |||
| import { buildWhereCondition } from "../graphql/schemas/utils/filters-kysely.js"; | |||
| import { CachingDatabase } from "../types/kyselySupabaseCaching.js"; | |||
| import { DataDatabase } from "../types/kyselySupabaseData.js"; | |||
There was a problem hiding this comment.
Might be way out of scope for this PR, but generally, the names Caching Database and Data Database are a bit unwieldy and slightly confusing. Especially Data Database is making me twitch every time 😄 How about something like CoreData and ChainData? For me personally, Caching Database doesn't really tell me what I would find in there.
| >(tableName: T): QueryStrategy<DB, T> { | ||
| const strategy = this.strategies[tableName as keyof StrategyMapping]; | ||
| if (!strategy) | ||
| throw new Error(`No strategy found for table ${String(tableName)}`); |
There was a problem hiding this comment.
Would be cool to get a bit of a hint what to do when there's no strat found for the table.
| @@ -0,0 +1,121 @@ | |||
| import { Kysely } from "kysely"; | |||
There was a problem hiding this comment.
I wonder how useful these tests are. They're essentially just testing that the service is passing the args along.
Our services are difficult to test at the moment and we don't really have tests for them. However, when you make changes to the GraphQL schema and potentially introduce new entities, you'll also alway have to add code to the services and might go below the thresholds set. PR hypercerts-org#218 is going to address this problem and make the services as well as some of the GraphQL testable. This is in anticipation of the next commit which is going to introduce the collections entity as a top-level entity.
To improve the maintainability, readabiity and testability of the API codebase some major refactors were implemented.
The BaseSupabaseService is executes different strategies and applies where filtering and sorting.
Query Strategies and Factory were created for clear declaration of querying strategies
Tests were updated and basic tests for the services created. Including getting the graphql tests running with vitest