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

Separate business-logic from data-layer logic in core-database/postgres #2055

Merged
merged 22 commits into from Feb 5, 2019

Conversation

Projects
None yet
3 participants
@paroxysm
Copy link
Contributor

paroxysm commented Feb 3, 2019

Proposed changes

Resolves #2006

The PR introduces a hierarchical change to the core-database/postgres.

I've encapsulated the business logic, previously in ConnectionInterface and PostgresConnection, into a new class DatabaseService, implements IDatabaseService, which references a database specific implementation of the IDatabaseConnection interface via constructor injection.

IDatabaseConnection supplies the repos that DatabaseService uses in certain methods.

The plugin providing a IDatabaseConnection concrete class is responsible for instantiating a DatabaseService and supply a IDatabaseConnection concrete class, among with other dependencies such as IWalletManager

In theory, the plugin can also provide its own implementation of IWalletManager and such..

The implication is that we're now exposing an IDatabaseService implementation to be used by the other modules. Most of these modules don't care about the underlying data-layer and shouldn't.
The exception would be core-api/core-graphql/core-snapshots which are(oddly) defining repositories that are accessing postgres specific internals(I use 'any' type-casting to get past compilation errors but this tech-debt that should be addressed sooner than later). These external repos need to be relocated into IDatabaseConnection repos and w/e additional logic they were doing be merged.

It shouldn't be too hard to convert the search/query code these external repositories are using into a generic implementation on the database specific repos.

I've tested that my relay runs and syncs as usual. I haven't tested some other affected modules such as core-graphql/snapshot outside their tests

The next step would be to start improving the DatabaseService tests, now that we can stub easier.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 3, 2019

@paroxysm Thanks for submitting this pull request, a maintainer will get back to you shortly!

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 3, 2019

@faustbrian @supaiku0 @air1one - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 3, 2019

@paroxysm Your pull request doesn't follow our contribution guidelines. Please review and correct it.

@faustbrian faustbrian self-requested a review Feb 4, 2019

@@ -14,6 +14,7 @@ export const plugin: Container.PluginDescriptor = {
async register(container: Container.IContainer, options) {
const manager = new SnapshotManager(options);

return manager.make(container.resolvePlugin<PostgresConnection>("database"));
const connection = container.resolvePlugin<Database.IDatabaseService>("database").connection as any;

This comment has been minimized.

@faustbrian

faustbrian Feb 4, 2019

Collaborator

This causes some resolution issues. Try yarn dump:testnet --config ../core/src/config/testnet through the CLI to reproduce it.

TypeError: Cannot read property 'connection' of null
    at Object.register (./packages/core-snapshots/dist/plugin.js:15:63)

This comment has been minimized.

@paroxysm

paroxysm Feb 5, 2019

Author Contributor

@faustbrian I've solved the issue.

@paroxysm

This comment has been minimized.

Copy link
Contributor Author

paroxysm commented Feb 4, 2019

I'll take a look after work 👍🏿
EDIT:

I've introduced a factory function for instantiating the DatabaseService class. I didn't like core-database extenders having to know of and being able to override the business repos.

By using the factory function, the extenders only need to know of an IWalletManager and IDatabaseConnection. It makes sense that the former can be overridden for a different implementation that can also be database specific(Right now it uses an in-memory model, but perhaps a future one will persist the wallets in the database or use cache employing a db-backing)

Also, the factory function allows us to break-up parts of the DatabaseService implementation into separate chunks(the more complicated methods). Since they all come together via the constructor, extenders of the core-database would also need to know of those chunks too, which isn't is intended.

paroxysm added some commits Feb 5, 2019

fix: startup issue in core-snapshots-cli(apparently it supports booti…
…ng up w/o a db connection)

refactor: export a database-service factory for instantiating it. This prevents db flavor implementors from overriding the business repos which have expected implementations already.
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 5, 2019

@paroxysm The ci/circleci: test-node11-0 job is failing as of 0a091e13265aa380307cf1176da82322828ed654. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

paroxysm added some commits Feb 5, 2019

@faustbrian faustbrian changed the base branch from develop to 2.2 Feb 5, 2019

@faustbrian faustbrian merged commit 43db7c6 into ArkEcosystem:2.2 Feb 5, 2019

6 checks passed

ci/circleci: test-node10-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-2 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-2 Your tests passed on CircleCI!
Details
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 5, 2019

@paroxysm Your pull request has been merged and marked as tier 0. It needs a specialized, in-depth review before we can decide on its value.

@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 5, 2019

@paroxysm do you want to take care of extracting the repositories out of core-api/graphql?

@paroxysm

This comment has been minimized.

Copy link
Contributor Author

paroxysm commented Feb 5, 2019

Yea, I'll handle that

faustbrian added a commit that referenced this pull request Feb 7, 2019

vasild added a commit that referenced this pull request Feb 7, 2019

Merge remote-tracking branch 'ArkEcosystem/core/develop' into verify-…
…peer-state

* ArkEcosystem/core/develop:
  refactor(core-database): separate business-logic from data-layer logic (#2055)

@paroxysm paroxysm deleted the paroxysm:feat/refactor-core-database branch Feb 21, 2019

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