In 1315 create item db model #179

jonavellecuerdo · 2025-06-23T15:00:44Z

Purpose and background context

DSC requires a way to interact with a DynamoDB table for tracking the state of an "item" during workflow executions. The dsc.db.item module defines a PynamoDB model, which serves as a Pythonic interface to interact with a DynamoDB table.

How can a reviewer manually see the effects of these changes?

Review the added unit tests in test/test_db.py.

Includes new or updated dependencies?

YES - Adds pynamodb to packages.

Changes expectations for external applications?

NO

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/IN-1315

Developer

All new ENV is documented in README
All new ENV has been added to staging and production environments
All related Jira tickets are linked in commit message(s)
Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

The commit message is clear and follows our guidelines (not just this PR message)
There are appropriate tests covering any new functionality
The provided documentation is sufficient for understanding any new functionality introduced
Any manual tests have been performed or provided examples verified
New dependencies are appropriate or there were no changes

jonavellecuerdo · 2025-06-23T15:01:27Z

pyproject.toml

    "PLR0913", 
    "PLR0915",
    "PTH", 
-    "S320",


Ruff removed this rule.

Yes, they removed a bunch so I had to remove several #noqas in s3-bagit-validator, something for all of us to keep an eye out for as we update dependencies in various repos!

jonavellecuerdo · 2025-06-23T15:03:03Z

tests/conftest.py

    monkeypatch.setenv("SENTRY_DSN", "None")
    monkeypatch.setenv("WORKSPACE", "test")
    monkeypatch.setenv("AWS_REGION_NAME", "us-east-1")
+    monkeypatch.setenv("AWS_DEFAULT_REGION", "us-east-1")


I think some AWS services use AWS_REGION_NAME and others use AWS_DEFAULT_REGION to set default region values. When running DSC in AWS, it will use the appropriate env var, which is why I didn't feel it was necessary to add AWS_DEFAULT_REGION to dsc.config.Config. 🤔

Why these changes are being introduced: * To establish idempotency for DSC workflows, it was decided that a DynamoDB table would be used to track the state of an "item" during workflow executions. The dsc.db.item module defines a PynamoDB model, which serves as a Pythonic interface to interact with the proposed DynamoDB table. How this addresses that need: * Define 'ItemDB' model using pynamodb.models.Model Side effects of this change: * DSC CLI commands will need to be updated to include required read/write operations to DynamoDB table. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1315

coveralls · 2025-06-23T15:13:16Z

Pull Request Test Coverage Report for Build 15880254053

Details

55 of 61 (90.16%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.4%) to 95.313%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dsc/db/models.py	51	53	96.23%
dsc/config.py	2	6	33.33%

Totals
Change from base Build 15834496448:	-0.4%
Covered Lines:	793
Relevant Lines:	832

💛 - Coveralls

ehanson8

Looks great! A few comments

ehanson8 · 2025-06-23T17:08:56Z

pyproject.toml

    "PLR0913", 
    "PLR0915",
    "PTH", 
-    "S320",


Yes, they removed a bunch so I had to remove several #noqas in s3-bagit-validator, something for all of us to keep an eye out for as we update dependencies in various repos!

dsc/db/item.py

ehanson8 · 2025-06-23T17:19:03Z

dsc/db/item.py

+    submit_attempts = NumberAttribute(default_for_new=0)
+    ingest_attempts = NumberAttribute(default_for_new=0)


Somewhere we should be specific about the CLI command connection between finalize and ingest_attempts given the differing vocabulary. Maybe just README.md? And submit_attempts may not really end up ever mattering but I guess it doesn't hurt to include it

Noted! I think this can also be explained visa updated docstrings for the base Workflow's submit and process_ingest_result methods as well.

ehanson8 · 2025-06-23T17:20:03Z

dsc/db/item.py

+        cls.Meta.table_name = table_name
+
+    @classmethod
+    def create(


Maybe create_submission or create_submission_row to be really specific?

Hmm, if we end up renaming the pynamodb model to include the phrase 'submission', I would opt to keep the method as create for now. 🤔 The naming convention also stems from the standard set of CRUD operations.

ehanson8 · 2025-06-23T17:21:35Z

dsc/db/item.py

+                condition=cls.item_identifier.does_not_exist()
+                & cls.batch_id.does_not_exist()
+            )
+        except PutError as exception:


Is this only raised in the case of duplicates or could a PutError be raised for different reasons? This seems really specific to just duplicates

I think that's a great question, I'm curious too.

If it's not specific to only duplicates, maybe there is something in the PutError that can be used to confirm the exception is related to duplicates and thus re-raising a custom ValueError makes sense?

Might also be worth defining a custom exception like ItemExistsError (or SubmissionExistsError if going that naming route) which could be raised. That would be unambiguous downstream what's happening if encountered, where ValueError could be lots of things.

The idea is that when DSC first writes an item submission to the DynamoDB table, the calling method (the reconcile command) would call ItemDB.create(). However, for updates to rows in the table, the built-in pynamodb.models.Model.update method would be used instead (by submit and finalize commands).

@ehanson8 Yes, this will be raised in the case of duplicates, but a PutError can be raised for any failing conditions.

I like the idea proposed by @ghukill to define a custom exception (db.exceptions) and can make the changes once we decide on the name of the "item" pynamodb model! 🤓

This has been addressed in the latest commit.

ghukill

Left a couple of comments and questions, but overall looking good to me.

tests/conftest.py

dsc/db/item.py

ghukill · 2025-06-23T18:22:52Z

tests/test_db.py

+    assert fetched_item.workflow_name == "workflow"
+
+
+def test_db_item_create_if_hash_key_and_range_key_exist_raise_error(mocked_item_db):


I might propose a test rename here, something like:

test_db_item_create_duplicate_item_identifier_and_batch_id_raise_error()

I think the item_identifier being the primary key and batch_id being the range key feel like implemenation details, but this test is kind of getting at identifer + batch is what makes something unique in the database.

Normally I'm pretty meh on test names, but this jumped out at me. Perhaps in a test docstring you could point out that item_identifier --> primary key and batch_id --> range key? and that the combination of the two is what gaurantees uniqueness?

Hmm, since we've renamed the DynamoDB model, the test can now be named:

test_db_itemsubmission_create_if_exists_raise_error()

What do you think?

dsc/db/item.py

ghukill · 2025-06-23T18:29:45Z

dsc/db/item.py

+                condition=cls.item_identifier.does_not_exist()
+                & cls.batch_id.does_not_exist()
+            )
+        except PutError as exception:


I think that's a great question, I'm curious too.

If it's not specific to only duplicates, maybe there is something in the PutError that can be used to confirm the exception is related to duplicates and thus re-raising a custom ValueError makes sense?

Might also be worth defining a custom exception like ItemExistsError (or SubmissionExistsError if going that naming route) which could be raised. That would be unambiguous downstream what's happening if encountered, where ValueError could be lots of things.

* Add detailed docstrings to ItemSubmissionDB * Rename DynamoDB model to 'ItemSubmissionDB' * Rename unit test * Raise custom exception when PutError is caused by 'ConditionalCheckFailedException'

dsc/db/models.py

jonavellecuerdo · 2025-06-25T14:37:15Z

dsc/db/models.py

+        workflow_name: str,
+        **attributes: Unpack[OptionalItemAttributes],
+    ) -> None:
+        """Create a new item (row) in the 'dsc-item-submissions' table.


"item" as in "DynamoDB item"

dsc/db/models.py

ghukill · 2025-06-25T14:39:56Z

dsc/db/models.py

+    item_identifier = UnicodeAttribute(range_key=True)
+    workflow_name = UnicodeAttribute()
+    dspace_handle = UnicodeAttribute(null=True)
+    status = UnicodeAttribute(null=True)


Noting for posterity what you shared @jonavellecuerdo, that pynamodb does not support an enum or list of accepted values. I think that's okay.

ghukill · 2025-06-25T14:41:20Z

dsc/db/models.py

+class ItemSubmissionStatus(StrEnum):
+    RECONCILE_SUCCESS = "reconcile_success"
+    RECONCILE_FAILED = "reconcile_failed"
+    SUBMIT_SUCCESS = "submit_success"
+    SUBMIT_FAILED = "submit_failed"
+    SUBMIT_MAX_RETRIES_REACHED = "submit_max_retries_reached"
+    INGEST_SUCCESS = "ingest_success"
+    INGEST_FAILED = "ingest_failed"
+    INGEST_UNKNOWN = "ingest_unknown"
+    INGEST_MAX_RETRIES_REACHED = "ingest_max_retries_reached"


+1 to defining the allowed statuses here. Do you have plans to use this elsewhere in code? Perhaps in the business logic when a particular status will get set?

Even if this commit / PR doesn't do that, I think this enum sets us up nicely for that.

ehanson8

Small docstring fix requested but otherwise this is great!

dsc/db/models.py

ehanson8

🎉

jonavellecuerdo commented Jun 23, 2025

View reviewed changes

jonavellecuerdo added 2 commits June 23, 2025 11:05

Add 'ITEM_TABLE_NAME' as required env var

bdb924a

jonavellecuerdo force-pushed the IN-1315-create-item-db-model branch from 83ab7ff to bdb924a Compare June 23, 2025 15:10

Update README

40acd55

jonavellecuerdo marked this pull request as ready for review June 23, 2025 15:14

jonavellecuerdo requested a review from a team as a code owner June 23, 2025 15:14

jonavellecuerdo requested review from ehanson8 and ghukill June 23, 2025 15:57

ehanson8 self-assigned this Jun 23, 2025

ehanson8 reviewed Jun 23, 2025

View reviewed changes

ghukill reviewed Jun 23, 2025

View reviewed changes

jonavellecuerdo changed the base branch from main to support-for-dynamodb June 23, 2025 20:33

Address comments in PR #179

c4049c7

* Add detailed docstrings to ItemSubmissionDB * Rename DynamoDB model to 'ItemSubmissionDB' * Rename unit test * Raise custom exception when PutError is caused by 'ConditionalCheckFailedException'

jonavellecuerdo commented Jun 25, 2025

View reviewed changes

dsc/db/models.py Show resolved Hide resolved

jonavellecuerdo commented Jun 25, 2025

View reviewed changes

jonavellecuerdo requested review from ehanson8 and ghukill June 25, 2025 14:37

ghukill approved these changes Jun 25, 2025

View reviewed changes

ehanson8 reviewed Jun 25, 2025

View reviewed changes

dsc/db/models.py Outdated Show resolved Hide resolved

dsc/db/models.py Show resolved Hide resolved

dsc/db/models.py Show resolved Hide resolved

fix docstring

7825d5b

jonavellecuerdo requested a review from ehanson8 June 25, 2025 15:14

ehanson8 approved these changes Jun 25, 2025

View reviewed changes

jonavellecuerdo merged commit 9d1a1ab into support-for-dynamodb Jun 25, 2025
2 checks passed

jonavellecuerdo deleted the IN-1315-create-item-db-model branch June 25, 2025 15:19

		submit_attempts = NumberAttribute(default_for_new=0)
		ingest_attempts = NumberAttribute(default_for_new=0)

		assert fetched_item.workflow_name == "workflow"


		def test_db_item_create_if_hash_key_and_range_key_exist_raise_error(mocked_item_db):

In 1315 create item db model #179

In 1315 create item db model #179

Uh oh!

Conversation

jonavellecuerdo commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose and background context

How can a reviewer manually see the effects of these changes?

Includes new or updated dependencies?

Changes expectations for external applications?

What are the relevant tickets?

Developer

Code Reviewer(s)

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!

coveralls commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15880254053

Details

💛 - Coveralls

Uh oh!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonavellecuerdo Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonavellecuerdo Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ghukill left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonavellecuerdo commented Jun 23, 2025 •

edited

Loading

coveralls commented Jun 23, 2025 •

edited

Loading

jonavellecuerdo Jun 24, 2025 •

edited

Loading

jonavellecuerdo Jun 24, 2025 •

edited

Loading