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

[JSON-API] Validate schema version & add minimal options for schema creation #10374

Merged
merged 24 commits into from
Jul 27, 2021

Conversation

realvictorprm
Copy link
Contributor

@realvictorprm realvictorprm commented Jul 22, 2021

should fix #10333

changelog_begin

  • [JSON-API] Schema versioning was introduced to the db schema. Because of this the field createSchema in the jdbcConfig was deprecated. Via the field start-mode you can specify:
    1. create-only: This is equal to the behaviour of createSchema=true so the db schema is created and then the application terminates.
    2. start-only: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the application terminates. This is also the replacement of createsSchema=false.
    3. create-if-needed-and-start: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the schema will be created/updated and the application proceeds with the startup.
    4. create-and-start: Similar to the first option but instead of terminating the application proceeds with the startup.

changelog_end

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@realvictorprm realvictorprm force-pushed the http-json/schema-versioning branch 2 times, most recently from 609983d to f4c6b7c Compare July 23, 2021 12:28
@realvictorprm realvictorprm requested a review from cocreature July 23, 2021 12:30
@realvictorprm
Copy link
Contributor Author

In theory we could also allow to add another case CreateOrUpdateAndTerminate for the use case of just checking whether it needs to be updated and if so updating it directly with suitable credentials.

Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty close, I think the biggest issue is that you broke backwards compatibility which needs to be fixed before merging. The rest is mostly style nitpicks

@realvictorprm realvictorprm force-pushed the http-json/schema-versioning branch from 446d3c1 to a1c395b Compare July 23, 2021 15:13
@realvictorprm realvictorprm requested a review from cocreature July 23, 2021 15:46
@realvictorprm realvictorprm force-pushed the http-json/schema-versioning branch 2 times, most recently from 06825e9 to efe80dc Compare July 26, 2021 11:21
@realvictorprm realvictorprm requested a review from cocreature July 26, 2021 12:28
@realvictorprm
Copy link
Contributor Author

Last open point is to fix the scala 2.12 build, will take another look into that after lunch.

Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!


sealed trait DbStartupResult
object DbStartupResult {
case object GracefullyExit extends DbStartupResult
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you really don’t seem to like my suggestion to move the logic for this to the call site 😁 fair enough I don’t care too much


private[this] val logger = ContextualizedLogger.get(getClass)

def fromStartupMode(dao: ContractDao, dbStartupMode: DbStartupMode)(implicit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment on the meaning of None and the difference between successfully returning None and throwing an exception. Looking at the code, it seems the difference is fairly arbitrary and the callsite handles both more or less the same so you could also consider if you really need to represent those two types of errors differently.

@@ -129,21 +131,23 @@ private[http] object JdbcConfig
s"${indent}url -- JDBC connection URL,\n" +
s"${indent}user -- database user name,\n" +
s"${indent}password -- database user password,\n" +
s"${indent}createSchema -- boolean flag, if set to true, the process will re-create database schema and terminate immediately.\n" +
s"${indent}createSchema -- boolean flag, if set to true, the process will re-create database schema and terminate immediately. This is deprecated and replaced by start-mode, however if set it will always overrule start-mode.\n" +
s"${indent}start-mode -- option setting how the schema should be handled. Valid options are ${DbStartupMode.allConfigValues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S11001001 is there some logic behind where we use camelCase and where we use kebab-case?
It seems like currently the option names are all kebab-case but then in the option-value we use camelCase?

If it was just the start mode, I’d say switching to kebab-case would make sense since then we’re consistent everywhere but it’s not. The websocket config still uses camelCase.

@realvictorprm
Copy link
Contributor Author

@cocreature OK, I decoupled the code a bit more, improved the tests further & made sure that all exceptions are properly handled with correct exit codes.

realvictorprm and others added 11 commits July 27, 2021 14:27
…SchemaHandlingResult.scala

Co-authored-by: Moritz Kiefer <[email protected]>
…eateSchema=true)

changelog_begin

- [JSON-API] Schema versioning was introduced to the db schema. Because of this the field `createSchema` in the jdbcConfig was deprecated. Via the field `start-mode` you can specify:
   1. `create-only`: This is equal to the behaviour of `createSchema=true` so the db schema is created and then the application terminates.
   2. `start-only`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the application terminates. This is also the replacement of `createsSchema=false`.
   3. `create-if-needed-and-start`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the schema will be created/updated and the application proceeds with the startup.
   4. `create-and-start`: Similar to the first option but instead of terminating the application proceeds with the startup.

changelog_end
@realvictorprm realvictorprm force-pushed the http-json/schema-versioning branch from 83fd4b4 to 178be8b Compare July 27, 2021 13:32
currentSchemaVersion = jdbcDriver.queries.schemaVersion
getVersionResult <- dao.transact(jdbcDriver.queries.version()).attempt
} yield getVersionResult
.fold(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably for a separate PR: maybe consider returning the error in an Either instead of returning None. That makes it very clear what the meaning of Left is where as None can be anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal :) I hesitated to go with an Either[Unit, _] 😅 But I agree that with Option is is more confusing right now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either[Throwable, _] should also work I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted that small nit with my last build fix :) Hopefully the build finally passes 😄

@victormueller-da victormueller-da merged commit c97cbca into main Jul 27, 2021
@victormueller-da victormueller-da deleted the http-json/schema-versioning branch July 27, 2021 16:28
azure-pipelines bot pushed a commit that referenced this pull request Jul 28, 2021
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@remyhaemmerle-da is in charge of this release.

Commit log:
```
b5e9d86 Moved DamlSubmission* into separate proto file [KVL-980] (#10362)
0da814d Let stable packages bypass LF version restrictions. (#10377)
c97cbca [JSON-API] Validate schema version & add minimal options for schema creation (#10374)
ebb8fab Add a ContractDao benchmark for payload queries (#10426)
85af078 LF: parser for LF versions (#10424)
b976c9c Disable autocommit for hikariCP for http-json-api db conn (#10427)
3ca46a4 Removed unused import. (#10425)
4d12493 Introduce buf checks [KVL-980] (#10411)
1c4ae50 Revert "Upgrade hikari to latest jdk8 version (#10406)" (#10421)
fe1b642 Don't fail if logs directory exists already (#10423)
7c88b56 participant-integration-api: Fix completion debug log output. (#10415)
fc305e6 [JSON-API] Shutdown on startup if the db connection is invalid (#10360)
6db5869 Update vcredist (#10417)
4b55f1a Connection pool for Contract Dao (#10359)
72cf2f3 LF: replace bazel keyword stable by default (#10410)
2bdcb7b update NOTICES file (#10414)
39c6e0b Fix oracle message too long error (#10413)
2094e24 Indexer ValidateAndWaitOnly startup mode for canton participant HA (#10290)
ad13a86 Windows dev-env Powershell 7 compatibility (#10408)
3cedd83 Easy to parse ledger-api-bench-tool logs (#10320)
16ff20c Fix links in changelog (#10409)
b325e8a participant-state: Remove `WriteService#rejectSubmission`. (#10407)
9be520c Keep the participant-state API prefixed. [KVL-1002] (#10405)
d88dc71 Upgrade hikari to latest jdk8 version (#10406)
8b337bd Publish ledger-indexer-benchmark (#10401)
9e05f38 ledger-api-domain: Store the deduplication duration in `Commands`. [KVL-1002] (#10403)
9c064da Allow imports of internal modules (#10397)
b9518ce participant-state-metrics: Add wrappers for v2. (#10404)
c3a3d60 don't call Gary, he's on holiday (#10400)
221d0a0 use doobie 0.9.0 Fragment-in-Fragment interpolation in json-api db-backend (#10399)
17709b5 use a single SQL query for any number of json-api query pairs (#10344)
6a16684 Stop publishing the db-backend artifact (#10396)
1bed05f Treat KeyWithMaintainers field structurally in all node types. (#10392)
d7077e1 Introduce locally-defined `Rejection` reasons instead of the participant-state rejection reason type. [KVL-1002] (#10376)
96f0483 [Divulgence pruning] Conformance tests implementation [DPP-484] (#10385)
28c5e9a update NOTICES file (#10386)
3879452 fix cut&paste typo bug; there are no insig lookup tweaks! (#10389)
7df9758 Daml export: make paths relative to daml.yaml (#10388)
90c3582 treat exerciseResult structurally for isReplayedBy (#10381)
27a0c69 Stop swallowing error message in non-repudiation conformance tests (#10387)
22b6101 rotate release duty after 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10346)
5242e2c LF: drop old serializability check for Values (#10382)
ee75530 LF: Specify nesting constraint for serialized values. (#10375)
4a33c03 LF: Add check of nesting in SValue.toValue (#10370)
91529ee clear up record specifications in http-json integration tests (#10366)
e8247af update compat versions for 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10354)
74751ba Populate workflow-id in the test-tool scenarios (#10372)
da9f8e3 clean-up perf tests (#10355)
42b70ad Fetch actingParties always non-empty for supported versions (>=1.6) (#10357)
37ff1a6 ledger-configuration: Return a structured error from checkTime. [KVL-1002] (#10373)
66284c1 Limit length of package ids to 64 characters (#10368)
a56cfea even earlier mount failure detection (#10371)
1bc0ccd update NOTICES file (#10367)
9c9b91e Support deletion of a large number of contracts (#10353)
1b5f99e Stop printing stacktrace on logging setup failures (#10364)
c0a24fe HA PoC behind a feature flag [DPP-426] (#10227)
63739fa Add conformance test for deeply nested values (#10319)
faf479e LF: add context in LookupError (#10314)
68dcda0 Drop unused textType from JSON API Oracle queries (#10356)
49745f6 Re-add `application_id` to `DamlCommandDedupKey` [KVL-1000] (#10341)
27d439d LF: Compile netsed ELets using constant stack (#10337)
628aa22 kvutils: Refined transaction validation [KVL-1015] (#10066)
d1e84c8 participant-integration-api: Fill out stubs in ApiSubmissionServiceSpec. (#10349)
905d8ad remove duplicated rule in create-daml-app dlint (#10352)
bdc3e50 Separate ledger export related message from other kvutils messages [KVL-980] (#10343)
70e8ff4 participant-integration-api: De-spacify PostCommitValidationSpec. (#10350)
186d279 release 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10345)
e58c7ba fix template dot-files (#10342)
cf2b61e participant-state: Remove the aliases to `Ref.LedgerString`. (#10325)
8148137 [JSON-API] Include the logging context in the ledger client for consistent logging (#10332)
c9666c7 Drop unnecessary mutability from speedy OnLedger state (#10340)
60dd96a update NOTICES file (#10347)
```
Changelog:
```
[Integration Kit] Moved definitions of `DamlSubmission` and `DamlSubmissionBatch` to a separate proto file under the package `com.daml.ledger.participant.state.kvutils.wire`. In case you are directly referencing these messages you will have to update your imports.

- [JSON-API] Schema versioning was introduced to the db schema. Because of this the field `createSchema` in the jdbcConfig was deprecated. Via the field `start-mode` you can specify:
   1. `create-only`: This is equal to the behaviour of `createSchema=true` so the db schema is created and then the application terminates.
   2. `start-only`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the application terminates. This is also the replacement of `createsSchema=false`.
   3. `create-if-needed-and-start`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the schema will be created/updated and the application proceeds with the startup.
   4. `create-and-start`: Similar to the first option but instead of terminating the application proceeds with the startup.

- [JSON-API] The json api now correctly shutdowns at startup if the provided db connection is invalid in case of `createSchema=false`

simplify oracle migration scripts
- [Daml Compiler] Imports of internal modules from stable packages are
  no longer illegal. Previously, the compiler raised an error when it
  encountered imports of internal modules such as
  `DA.Internal.Template`. Such imports are now accepted by the compiler.
  Note, however, that internal modules are still not part of the stable
  API. Fixes #10379
[Integration kit] Extended the Ledger API test tool with tests for the pruning of all divulgence events.
- [Daml export] The generated paths to data-dependencies DALFs are now
  relative to the generated daml.yaml. Fixes
  #10378.
- [JSON API] Fix an error where transactions that delete a large
  number of contracts resulted in stackoverflows with the PostgreSQL
  backend and database errors with Oracle.
[Integration Kit] The command de-duplication key now also includes the daml application ID
* [Integration Kit] Made `daml_kvutils.proto`'s location follow its proto package and moved `LedgerExportEntry` into a separate proto file. You may have to update your proto import statements in case you are directly importing proto files from the kvutils library.

- [JSON-API] Connection tries from the json api to the ledger now include the logging context, more specifically the instance_uuid is included in each logging statement.

```

CHANGELOG_BEGIN
CHANGELOG_END
remyhaemmerle-da pushed a commit that referenced this pull request Jul 28, 2021
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@remyhaemmerle-da is in charge of this release.

Commit log:
```
b5e9d86 Moved DamlSubmission* into separate proto file [KVL-980] (#10362)
0da814d Let stable packages bypass LF version restrictions. (#10377)
c97cbca [JSON-API] Validate schema version & add minimal options for schema creation (#10374)
ebb8fab Add a ContractDao benchmark for payload queries (#10426)
85af078 LF: parser for LF versions (#10424)
b976c9c Disable autocommit for hikariCP for http-json-api db conn (#10427)
3ca46a4 Removed unused import. (#10425)
4d12493 Introduce buf checks [KVL-980] (#10411)
1c4ae50 Revert "Upgrade hikari to latest jdk8 version (#10406)" (#10421)
fe1b642 Don't fail if logs directory exists already (#10423)
7c88b56 participant-integration-api: Fix completion debug log output. (#10415)
fc305e6 [JSON-API] Shutdown on startup if the db connection is invalid (#10360)
6db5869 Update vcredist (#10417)
4b55f1a Connection pool for Contract Dao (#10359)
72cf2f3 LF: replace bazel keyword stable by default (#10410)
2bdcb7b update NOTICES file (#10414)
39c6e0b Fix oracle message too long error (#10413)
2094e24 Indexer ValidateAndWaitOnly startup mode for canton participant HA (#10290)
ad13a86 Windows dev-env Powershell 7 compatibility (#10408)
3cedd83 Easy to parse ledger-api-bench-tool logs (#10320)
16ff20c Fix links in changelog (#10409)
b325e8a participant-state: Remove `WriteService#rejectSubmission`. (#10407)
9be520c Keep the participant-state API prefixed. [KVL-1002] (#10405)
d88dc71 Upgrade hikari to latest jdk8 version (#10406)
8b337bd Publish ledger-indexer-benchmark (#10401)
9e05f38 ledger-api-domain: Store the deduplication duration in `Commands`. [KVL-1002] (#10403)
9c064da Allow imports of internal modules (#10397)
b9518ce participant-state-metrics: Add wrappers for v2. (#10404)
c3a3d60 don't call Gary, he's on holiday (#10400)
221d0a0 use doobie 0.9.0 Fragment-in-Fragment interpolation in json-api db-backend (#10399)
17709b5 use a single SQL query for any number of json-api query pairs (#10344)
6a16684 Stop publishing the db-backend artifact (#10396)
1bed05f Treat KeyWithMaintainers field structurally in all node types. (#10392)
d7077e1 Introduce locally-defined `Rejection` reasons instead of the participant-state rejection reason type. [KVL-1002] (#10376)
96f0483 [Divulgence pruning] Conformance tests implementation [DPP-484] (#10385)
28c5e9a update NOTICES file (#10386)
3879452 fix cut&paste typo bug; there are no insig lookup tweaks! (#10389)
7df9758 Daml export: make paths relative to daml.yaml (#10388)
90c3582 treat exerciseResult structurally for isReplayedBy (#10381)
27a0c69 Stop swallowing error message in non-repudiation conformance tests (#10387)
22b6101 rotate release duty after 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10346)
5242e2c LF: drop old serializability check for Values (#10382)
ee75530 LF: Specify nesting constraint for serialized values. (#10375)
4a33c03 LF: Add check of nesting in SValue.toValue (#10370)
91529ee clear up record specifications in http-json integration tests (#10366)
e8247af update compat versions for 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10354)
74751ba Populate workflow-id in the test-tool scenarios (#10372)
da9f8e3 clean-up perf tests (#10355)
42b70ad Fetch actingParties always non-empty for supported versions (>=1.6) (#10357)
37ff1a6 ledger-configuration: Return a structured error from checkTime. [KVL-1002] (#10373)
66284c1 Limit length of package ids to 64 characters (#10368)
a56cfea even earlier mount failure detection (#10371)
1bc0ccd update NOTICES file (#10367)
9c9b91e Support deletion of a large number of contracts (#10353)
1b5f99e Stop printing stacktrace on logging setup failures (#10364)
c0a24fe HA PoC behind a feature flag [DPP-426] (#10227)
63739fa Add conformance test for deeply nested values (#10319)
faf479e LF: add context in LookupError (#10314)
68dcda0 Drop unused textType from JSON API Oracle queries (#10356)
49745f6 Re-add `application_id` to `DamlCommandDedupKey` [KVL-1000] (#10341)
27d439d LF: Compile netsed ELets using constant stack (#10337)
628aa22 kvutils: Refined transaction validation [KVL-1015] (#10066)
d1e84c8 participant-integration-api: Fill out stubs in ApiSubmissionServiceSpec. (#10349)
905d8ad remove duplicated rule in create-daml-app dlint (#10352)
bdc3e50 Separate ledger export related message from other kvutils messages [KVL-980] (#10343)
70e8ff4 participant-integration-api: De-spacify PostCommitValidationSpec. (#10350)
186d279 release 1.16.0-snapshot.20210720.7404.0.b7cf42d1 (#10345)
e58c7ba fix template dot-files (#10342)
cf2b61e participant-state: Remove the aliases to `Ref.LedgerString`. (#10325)
8148137 [JSON-API] Include the logging context in the ledger client for consistent logging (#10332)
c9666c7 Drop unnecessary mutability from speedy OnLedger state (#10340)
60dd96a update NOTICES file (#10347)
```
Changelog:
```
[Integration Kit] Moved definitions of `DamlSubmission` and `DamlSubmissionBatch` to a separate proto file under the package `com.daml.ledger.participant.state.kvutils.wire`. In case you are directly referencing these messages you will have to update your imports.

- [JSON-API] Schema versioning was introduced to the db schema. Because of this the field `createSchema` in the jdbcConfig was deprecated. Via the field `start-mode` you can specify:
   1. `create-only`: This is equal to the behaviour of `createSchema=true` so the db schema is created and then the application terminates.
   2. `start-only`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the application terminates. This is also the replacement of `createsSchema=false`.
   3. `create-if-needed-and-start`: With this the schema version is checked and if no version or an version was found which is not equal to the internal schema version then the schema will be created/updated and the application proceeds with the startup.
   4. `create-and-start`: Similar to the first option but instead of terminating the application proceeds with the startup.

- [JSON-API] The json api now correctly shutdowns at startup if the provided db connection is invalid in case of `createSchema=false`

simplify oracle migration scripts
- [Daml Compiler] Imports of internal modules from stable packages are
  no longer illegal. Previously, the compiler raised an error when it
  encountered imports of internal modules such as
  `DA.Internal.Template`. Such imports are now accepted by the compiler.
  Note, however, that internal modules are still not part of the stable
  API. Fixes #10379
[Integration kit] Extended the Ledger API test tool with tests for the pruning of all divulgence events.
- [Daml export] The generated paths to data-dependencies DALFs are now
  relative to the generated daml.yaml. Fixes
  #10378.
- [JSON API] Fix an error where transactions that delete a large
  number of contracts resulted in stackoverflows with the PostgreSQL
  backend and database errors with Oracle.
[Integration Kit] The command de-duplication key now also includes the daml application ID
* [Integration Kit] Made `daml_kvutils.proto`'s location follow its proto package and moved `LedgerExportEntry` into a separate proto file. You may have to update your proto import statements in case you are directly importing proto files from the kvutils library.

- [JSON-API] Connection tries from the json api to the ledger now include the logging context, more specifically the instance_uuid is included in each logging statement.

```

CHANGELOG_BEGIN
CHANGELOG_END

Co-authored-by: Azure Pipelines DAML Build <[email protected]>
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.

Validate JSON API schema
3 participants