Updates to CLI and programatic API #2474

Niels-b · 2025-11-13T09:35:03Z

Updated contract API to allow only a single contract file. Includes preparatory changes to publish API.

Tried to cover as much from the CLI and API wrt to contract yaml files as possible. If this is too much for now, or not fully complete. Please let me know!

LaurenDebruyn

Please add more information to your PR. E.g. that the API needs to be backwards compatible. It's a lot of effort to derive this from the code itself (I already created multiple comments before I noticed this :D)
I would also prevent changing the existing API (e.g. renaming verify_contracts_locally to verify_contract_locally breaks backwards compatibility). Instead I would mark it as deprecated (we can still raise an exception when you try to verify multiple contracts, although this breaks backwards compatibility as well), and create a new method verify_contract_locally.
We should create a follow-up PR where we remove the logic for multiple contracts from the rest of the code (including ContractVerificationSessionResult).

LaurenDebruyn · 2025-11-13T13:44:32Z

soda-core/src/soda_core/common/soda_cloud.py

                    soda_cloud_file_id=response_json.get("fileId", None),
                ),
                soda_qualified_dataset_name=contract_yaml.dataset,
+                dataset_id=response_json.get("datasetId", None),


Could you do the same for the verify response? Or was there a specific reason why you want to get this from the checks?

Good suggestion, but out of scope for this PR. We will tackle that later

soda-core/src/soda_core/contracts/api/publish_api.py

soda-core/src/soda_core/contracts/api/test_api.py

soda-core/src/soda_core/contracts/api/verify_api.py

LaurenDebruyn · 2025-11-13T13:59:39Z

soda-core/src/soda_core/contracts/api/verify_api.py



-def verify_contracts_locally(
+def verify_contract_locally(


Either we are backwards compatible, or we are not. But renaming the function makes it per definition not backwards compatible IMO.
My proposal would be that you keep the existing function as is and mark it as deprecated (can you easily do this in Python?). We can still raise an exception if you try to verify more than one contract at once. I would create an entirely new method for the singular flow though.

I tend to agree here. Split into two methods, keeping the original signature in one, introducing new signature in the other.

The old method would then perform the additional checks (length of the contract_file_paths array should be one).

soda-core/src/soda_core/contracts/api/verify_api.py

LaurenDebruyn · 2025-11-13T14:18:28Z

soda-core/src/soda_core/contracts/api/verify_api.py

+        # TODO: change this when we fully deprecate a list of contract file paths
+        # This is only here to make sure the rest of the code still works.


Are you planning a follow-up PR for this? I wouldn't keep this code around for too long because we don't use multiple contracts anymore.

Indeed, technically we don't support lists anymore. You could do the inverse here:

if isinstance(contract_file_path, list): contract_file_path = contract_file_path[0] else: contract_file_path = None

Doesn't have to part of this PR, but ideally soon-ish, as we will forget about this :-)

Not in scope for now. But I agree that this should definitely be done!

soda-core/src/soda_core/contracts/contract_publication.py

soda-core/src/soda_core/contracts/api/verify_api.py

nielsn · 2025-11-14T09:19:21Z

soda-core/src/soda_core/contracts/api/verify_api.py

+        # TODO: change this when we fully deprecate a list of contract file paths
+        # This is only here to make sure the rest of the code still works.


Indeed, technically we don't support lists anymore. You could do the inverse here:

if isinstance(contract_file_path, list): contract_file_path = contract_file_path[0] else: contract_file_path = None

Doesn't have to part of this PR, but ideally soon-ish, as we will forget about this :-)

soda-core/src/soda_core/contracts/api/verify_api.py

soda-core/src/soda_core/contracts/api/test_api.py

soda-core/src/soda_core/contracts/api/publish_api.py

…hanges to publish API

… identifier. Exposed publish_contract to API

Niels-b · 2025-11-14T16:25:23Z

Note: before merging still check the publish flow with new BE changes!

sonarqubecloud · 2025-11-17T10:39:44Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Niels-b · 2025-11-17T10:39:48Z

Update: testing revealed that the dataset_id was not parsed correctly from the response. The latest commit fixes that.
PR should be ready now.

LaurenDebruyn

Good work!
I've added a few NITs for readability, but the overall code looks much better now :)

LaurenDebruyn · 2025-11-17T10:41:42Z

soda-core/src/soda_core/cli/cli.py

        type=str,
-        nargs="+",
-        help="One or more contract file paths. Use this to work with local contracts.",
+        help="One contract file path. Use this to work with local contracts.",


NIT: I wouldn't emphasize the one

Suggested change

help="One contract file path. Use this to work with local contracts.",

help="Contract file path to verify. Use this to work with local contracts.",

LaurenDebruyn · 2025-11-17T10:42:06Z

soda-core/src/soda_core/cli/cli.py

 def _setup_contract_publish_command(contract_parsers) -> None:
    publish_parser = contract_parsers.add_parser("publish", help="Publish a contract")
-    publish_parser.add_argument("-c", "--contract", type=str, nargs="+", help="One or more contract file paths.")
+    publish_parser.add_argument("-c", "--contract", type=str, help="One contract file path.")


Same here

Suggested change

publish_parser.add_argument("-c", "--contract", type=str, help="One contract file path.")

publish_parser.add_argument("-c", "--contract", type=str, help="Contract file path to verify.")

LaurenDebruyn · 2025-11-17T10:46:59Z

soda-core/src/soda_core/contracts/api/verify_api.py

    check_paths: Optional[list[str]] = None,
    dwh_data_source_file_path: Optional[str] = None,
+) -> ContractVerificationSessionResult:
+    if not isinstance(contract_file_paths, list):


NIT: I would check for str instead of list to make it easier to read:

Suggested change

if not isinstance(contract_file_paths, list):

if isinstance(contract_file_paths, str):

LaurenDebruyn · 2025-11-17T10:47:36Z

soda-core/src/soda_core/contracts/api/verify_api.py

    verbose: bool = False,
    blocking_timeout_in_minutes: int = 60,
+) -> ContractVerificationSessionResult:
+    if not isinstance(contract_file_paths, list):


Same remark here:

Suggested change

if not isinstance(contract_file_paths, list):

if isinstance(contract_file_paths, str):

LaurenDebruyn · 2025-11-17T10:50:17Z

soda-core/src/soda_core/contracts/api/verify_api.py

+        # TODO: change this when we fully deprecate a list of contract file paths
+        # This is only here to make sure the rest of the code still works.
+        if isinstance(contract_file_path, str):
+            contract_file_paths = [contract_file_path]
+        else:
+            contract_file_paths = None


NIT: move this up next to the dataset_identifier backwards compatibility code block so that we have these workarounds nicely grouped together.

Niels-b requested review from m1n0 and nielsn November 13, 2025 10:57

Niels-b marked this pull request as ready for review November 13, 2025 10:57

LaurenDebruyn requested changes Nov 13, 2025

View reviewed changes

nielsn reviewed Nov 14, 2025

View reviewed changes

Niels-b requested review from LaurenDebruyn and nielsn November 14, 2025 16:09

Niels-b added 5 commits November 14, 2025 17:10

Updated contract API to allow only a single contract file. Includes c…

1446c11

…hanges to publish API

More updates to contract API

ce90bd9

Implemented PR feedback: re-introduced old functions, changed dataset…

15ca49f

… identifier. Exposed publish_contract to API

Fix rogue dataset_identifiers arguments

2c94e77

Change deprecate import

cd35de0

Niels-b force-pushed the r-410-expose-cli-publish-command-and-return-dataset-id branch from debf0e7 to cd35de0 Compare November 14, 2025 16:10

Fix dataset_id being None in the results

a3dd48d

LaurenDebruyn approved these changes Nov 17, 2025

View reviewed changes

		# TODO: change this when we fully deprecate a list of contract file paths
		# This is only here to make sure the rest of the code still works.

	help="One contract file path. Use this to work with local contracts.",
	help="Contract file path to verify. Use this to work with local contracts.",

	publish_parser.add_argument("-c", "--contract", type=str, help="One contract file path.")
	publish_parser.add_argument("-c", "--contract", type=str, help="Contract file path to verify.")

	if not isinstance(contract_file_paths, list):
	if isinstance(contract_file_paths, str):

Updates to CLI and programatic API #2474

Are you sure you want to change the base?

Updates to CLI and programatic API #2474

Uh oh!

Conversation

Niels-b commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurenDebruyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Niels-b commented Nov 14, 2025

Uh oh!

sonarqubecloud bot commented Nov 17, 2025

Quality Gate passed

Uh oh!

Niels-b commented Nov 17, 2025

Uh oh!

LaurenDebruyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Niels-b commented Nov 13, 2025 •

edited

Loading