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

Complete PersistentMembershipState mappings #33

Open
wants to merge 4 commits into
base: release/1.1
Choose a base branch
from

Conversation

manosbatsis
Copy link
Contributor

@manosbatsis manosbatsis commented Nov 3, 2020

Updated PersistentMembershipState and with additional constraints and state member mappings:

  • Added non-null constraints to PersistentMembershipState's cordaIdentity, networkId, status
  • Added PersistentMembershipState issuer, issued and modified to expose as query criteria
  • Added nullable PersistentMembershipState businessIdentity based on the BNIdentity?.toString() implementation used, to expose as query criteria
  • Added liquibase changesets for the above

I state that this PR is in accordance with the Developer's Certificate of Origin.

@bpaunescu
Copy link
Contributor

@manosbatsis Appreciate you taking an interest in making this a better product. May I ask why is there a need for this additional query criteria? I would expect there to also be a change to the query service to justify these updates.

Regarding the nullable=false, that' not required since the data types are non-nullable.

@manosbatsis
Copy link
Contributor Author

Hi @bpaunescu , thanks for your reply. We're trying to convince a couple of clients to migrate from the previous bn-apps and adopt the extension. They need the ability to use VaultCustomQueryCriteria that correspond to any members of MembershipState.

Indeed nullable=false seems redundant from a cordapp developer's point of view, but our clients have database administrators that love code audits and the like, so those were added for consistency between the Kotlin and relational model.

For now we use a custom service as the one provided with the extension seems to assume a single result per networkId and Party combination. That said, i would be more than happy to discuss/contribute for any agreed improvements on the service API.

@bpaunescu
Copy link
Contributor

@manosbatsis Indeed, this solution was never intended to be C.I. or Accounts aware but that doesn't mean it shouldn't. I will have to discuss this a bit with product before moving forward.

One thing to keep in mind, we're in the middle of the QA part of the Q4 release and this contribution might not make the official release even if merged in the next few days. I hope that doesn't mean you can't make use of it.

In the meantime, @tubibuto what do you think? Opinions?

@tubibuto
Copy link
Contributor

I think code-wise solution looks good, maybe I would put this new change-set to a new migration script instead being in initial one. Did you test DB migration from 1.0 to 1.1 or you just did straight schema initialisation to 1.1 version ?

I agree with @bpaunescu that this probably won't make the cut in 1.1 official release but it still can be a beneficial addition to the open source code which can be checked out and used by customers.

@manosbatsis
Copy link
Contributor Author

Thanks @tubibuto. What would be the file name for the new migration script file? Only did a straight schema initialisation to 1.1, perhaps there's some way to provide a test for the migration? Haven't done this before but if you guys could point to an example i'd give it a try.

@tubibuto
Copy link
Contributor

Thanks @tubibuto. What would be the file name for the new migration script file? Only did a straight schema initialisation to 1.1, perhaps there's some way to provide a test for the migration? Haven't done this before but if you guys could point to an example i'd give it a try.

Maybe something like membership-state-schema-v1.changelog-additional-fields.xml ?

I would recommend doing it manually:

  1. Take 1.0 jars and run them on node (you will need to run migration explicitly if you are on Corda 4.6 or higher)
  2. Take jars you build and run nodes with them

If that works, we should be fine with that migration path.

@manosbatsis
Copy link
Contributor Author

@tubibuto @bpaunescu Updated based on comments above and also tested against CE 4.6.1 using run-migration-scripts -app-schemas, although i had to temporarily set bn-extension's cordaPlatformVersion to 8. Is platform v9 really necessary? Anyway, the log says the migration of the additional changeset file succeeded::


[INFO ] 2020-11-30T10:29:15,017Z [main] databaseInitialisation. - DatabaseInitialisation(id="EKzITU2W";changeset_count="1") {}
[INFO ] 2020-11-30T10:29:15,017Z [main] databaseInitialisation. - DatabaseInitialisation(id="EKzITU2W";changeset="migration/membership-state-schema-v1.changelog-additional-fields.xml::complete_initial_membership_state_columns_and_constraints::R3 Corda";source="business-networks-contracts-1.1-SNAPSHOT.jar";status="to be run") {}
[INFO ] 2020-11-30T10:29:15,018Z [main] databaseInitialisation. - defaultSchemaName=PUBLIC, liquibaseSchemaName=PUBLIC, outputDefaultSchema=true, objectQuotingStrategy=LEGACY {}
[INFO ] 2020-11-30T10:29:15,021Z [main] databaseInitialisation. - DatabaseInitialisation(id="EKzITU2W";changeset="migration/membership-state-schema-v1.changelog-additional-fields.xml::complete_initial_membership_state_columns_and_constraints::R3 Corda";source="business-networks-contracts-1.1-SNAPSHOT.jar";status="start") {}
[INFO ] 2020-11-30T10:29:15,060Z [main] databaseInitialisation. - DatabaseInitialisation(id="EKzITU2W";changeset="migration/membership-state-schema-v1.changelog-additional-fields.xml::complete_initial_membership_state_columns_and_constraints::R3 Corda";source="business-networks-contracts-1.1-SNAPSHOT.jar";status="successful") {}
[INFO ] 2020-11-30T10:29:15,073Z [main] internal.Node. - Connected to H2 database. {}
[INFO ] 2020-11-30T10:29:15,073Z [main] databaseInitialisation. - DatabaseInitialisation(id="EKzITU2W";status="successful") {}
[INFO ] 2020-11-30T10:29:15,073Z [main] BasicInfo. - Database migration scripts for core and app schemas complete. There are no outstanding database changes. {}

@manosbatsis manosbatsis mentioned this pull request Nov 30, 2020
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