-
Notifications
You must be signed in to change notification settings - Fork 37
Factory reset from fixtures #812
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
base: feature/634-boostrapping-fdp
Are you sure you want to change the base?
Conversation
using TreeSet to ensure uniqueness and lexicographic ordering
and DRY usage of removeFromFixtureHistory
MarekSuchanek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻 I like this approach... it should be then in docs that resetting works only with those "default" fixture files/resources (so it is clear that in some use cases you might want to override those and in some create separate files).
As a side effect, reset might apply new fixture resources that were not there previously 🤔 but I guess that does not matter as it would happen upon any restart anyway, right?
|
|
||
| private void clearResourceDefinitions() { | ||
| log.debug("Clearing resource definitions"); | ||
| resourceDefinitionChildMetadataRepository.deleteAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these added to make sure nothing is left out? Otherwise, cascading delete should work I believe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MarekSuchanek thanks for the review! :)
You're right, the deleteAll() calls for metadataSchemaUsageRepository, resourceDefinitionChildMetadataRepository, andresourceDefinitionLinkRepository are redundant.
However, line 188 (resourceDefinitionChildRepository.deleteAll()) does seem to be required, for some reason.
Still, I like to be explicit here, and not rely on cascades.
Note
In my experience cascade deletes, in general, can lead to very nasty surprises.
One example of cascade deletes causing confusion is in #802, where it turned out that overriding the default Einstein user account in a test fixture caused the related objects to be deleted as well.
Hi @MarekSuchanek , yes you're right that the current approach only works with the default fixture files, and that could lead to problems later on... 🤔 One of the challenges is that we cannot know how people are going to name their fixture override files and which entities they will contain. I do see a few options to mitigate that:
To expand upon option 3., we could distribute our default fixtures over the following required subfolders, where the subfolder names match the names of the relevant
and then adapt What do you think? |
|
To expand upon option 4.: It may be easier to introduce a naming convention for fixtures files, as in or as regex ^(?<order>[0-9]{4})_(?<package>[a-z]+)_(?<description>[a-zA-Z0-9\-]+)\.json$where
with the restriction that the fixtures in each file must belong to the specified package. EDIT: this idea is worked out in the following commits |
…n pattern
matching a regex like ^(?<order>[0-9]{4})_(?<package>[a-z]+)_(?<description>[a-zA-Z0-9\-]+)\.json$
and remove the explicit filename tests, because this is now implicitly done during population before each test
todo: adapt tests
04d0073 to
ca112b1
Compare
and same for corresponding tests to improve cohesion
7ebe519 to
c253a56
Compare
|
Indeed the option 4 looks the best, although it still needs to be documented and naturally expects that the name will correspond with contents. But we should expect people adding/overriding the fixtures to be knowledgeable about such things. It is a great solution. Wrt the TODOs in comments, I agree with moving the methods to |
This reproduces the DataIntegrityViolationException 'null value in column target_resource_definition_id of relation resource_definition_child violates not-null constraint'
coming from FAIRDataPoint/src/main/java/org/fairdatapoint/service/membership/MembershipService.java Line 79 in 5f8fd38
|

Possible implementation of factory reset using repository populator.
The basic idea is that we remove the relevant entries from the fixture history, update the list of resources accordingly, and then run the populator again.
TODO:
src/main/resources/org/fairdatapoint/service/reset