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: Remove QueryFactory #343

Merged
merged 11 commits into from
Mar 7, 2023
Merged

refactor: Remove QueryFactory #343

merged 11 commits into from
Mar 7, 2023

Conversation

mitjat
Copy link
Contributor

@mitjat mitjat commented Mar 3, 2023

Follow-up to #341 now that it is no longer required to know the ChainID to query the DB.

This PR removes the QueryFactory (QF) abstraction. It was hardly needed any more after its main parameter (chainID) was rendered obsolete. The remaining uses of QueryFactory's runtime field were converted so that the runtime is passed into each query as an explicit parameter.

Benefits of killing QF:

  • reduces global-ish state (in QF)
  • reduces number of ways in which we pass parameters into queries
  • removes a layer of templated strings so SQL is easier to reason about and easier to copy-paste (for debugging etc)

A similar-to-QF lightweight templating approach remains in genesis.go. I didn't touch it because we'll majorly clean up its SQL generation (from manually stringifying args to using SQL templates) in an upcoming PR.

For reviewers: Code at most commits does not compile; I still left them there because they each represent a logical step, so it's easier to follow along (IMO).

Testing
I ran e2e_regressions; consensus and emerald ran for 100 blocks. None of the tested URLs found any differences.

storage/client/queries/queries.go Outdated Show resolved Hide resolved
storage/client/client.go Show resolved Hide resolved
analyzer/modules/consensusaccounts.go Outdated Show resolved Hide resolved
@mitjat mitjat force-pushed the mitjat/db-schema-no-oasis3 branch from 6f288b5 to c9a1636 Compare March 7, 2023 18:54
@mitjat mitjat force-pushed the mitjat/query-factory-kill branch from fa3cea2 to cd60eb1 Compare March 7, 2023 18:54
@mitjat mitjat force-pushed the mitjat/db-schema-no-oasis3 branch from c9a1636 to 77866be Compare March 7, 2023 19:08
Base automatically changed from mitjat/db-schema-no-oasis3 to main March 7, 2023 19:13
@mitjat mitjat force-pushed the mitjat/query-factory-kill branch from cd60eb1 to f27c76e Compare March 7, 2023 19:32
@mitjat mitjat enabled auto-merge March 7, 2023 19:32
@mitjat mitjat merged commit 4e7290e into main Mar 7, 2023
@mitjat mitjat deleted the mitjat/query-factory-kill branch March 7, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants