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

add new data model minter and migration file and tests #454

Closed
wants to merge 2 commits into from

Conversation

jsjiang
Copy link
Contributor

@jsjiang jsjiang commented Aug 7, 2023

@rushirajnenuji @datadavev Hi Dave and Rushiraj,
I refactored the minter data model a little bit and attached a few tests. Please review and let me know if you have questions.

Thank you

Jing

@jsjiang
Copy link
Contributor Author

jsjiang commented Aug 7, 2023

SQL query from the new '0003' migration file:

ezid % python manage.py sqlmigrate ezidapp 0003
--
-- Create model Minter
--
CREATE TABLE `ezidapp_minter` (`id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY, `prefix` varchar(255) NOT NULL UNIQUE, `minterState` json NOT NULL, `createTime` integer NOT NULL, `updateTime` integer NOT NULL);
--
-- Alter field status on binderqueue
--
--
-- Alter field status on crossrefqueue
--
--
-- Alter field status on datacitequeue
--
--
-- Alter field status on searchindexerqueue
--
CREATE INDEX `ezidapp_minter_createTime_dfbbf01f` ON `ezidapp_minter` (`createTime`);
CREATE INDEX `ezidapp_minter_updateTime_a91cebee` ON `ezidapp_minter` (`updateTime`);

Copy link
Contributor

@datadavev datadavev left a comment

Choose a reason for hiding this comment

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

The modified and create time stamp functionality needs to be checked. Should also try and add more tests for minter functionality to verify it operates correctly after restoration from the database.

# The time the identifier was created as a Unix timestamp. If not
# specified, the current time is used.
createTime = django.db.models.IntegerField(
default=default_time(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Here default is set to the value when default_time is executed, which is when the python interpreter loads the file, not when the class is instantiated. Should probably use the Django ORM mechanisms for auto timestamps, e.g. auto_now_add and auto_now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Django doc on Field.default

The default value for the field. This can be a value or a callable object. If callable it will be called every time a new object is created.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a callable if it was default=default_time, but as it is, you get a static value evaluated when the module is loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the createTime and updateTime are int, we cannot use auto timestamps. How about just set the default value to zero and have business logic to handle the real values?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, try it with default=default_time (i.e. without parentheses). It should work since it says that default is the callable default_time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default=default_time (i.e. without parentheses) worked! Thank you Dave.

# The time the minter was last updated as a Unix timestamp. If
# not specified, the current time is used.
updateTime = django.db.models.IntegerField(
default=default_time(),
Copy link
Contributor

Choose a reason for hiding this comment

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

As for createTime.

prefix='ark:/91101/r02/'
minter = ezidapp.models.minter.Minter.objects.create(prefix=prefix, minterState={})
t2 = minter.createTime
assert t2 >= t1
Copy link
Contributor

Choose a reason for hiding this comment

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

Here t2 should always be > t1. Suggest adding a brief delay (e.g. time.sleep(0.1)) before line 71.

Copy link
Contributor

@rushirajnenuji rushirajnenuji left a comment

Choose a reason for hiding this comment

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

During the dev-sync call with @jsjiang, we reviewed this and Jing made modifications as per @datadavev's suggestions. We tested the changes and this looks good.

@jsjiang
Copy link
Contributor Author

jsjiang commented Aug 8, 2023

@datadavev @rushirajnenuji Thank you Dave and Rushiraj for reviewing and providing feedback. I have changed the code to use default=default_time for both createTime and updateTime and have adjusted tests. Please review.

Thank you

Jing

@jsjiang
Copy link
Contributor Author

jsjiang commented Aug 9, 2023

Pack this change to the BDB minter migration package and release at the same time.

@jsjiang jsjiang closed this Aug 9, 2023
@jsjiang jsjiang deleted the 442_create_mysql_table_for_minter_v2 branch July 26, 2024 22:46
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