External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch #9984

qqmyers · 2023-10-06T22:11:56Z

What this PR does / why we need it: Per the issue, it adds missing db constraints and improves the code for registering new external vocab values by isolating the update in a small transaction and stopping any attempt to add a duplicate value from killing the overall dataset edit/other transaction being done.

Which issue(s) this PR closes:

Closes #9983

Special notes for your reviewer: As of now, we don't know why the constraints are not there. There is some dv-tech channel slack discussion about it but no conclusions.
If the db has duplicate values now, the current flyway script would cause install failure. However, it appears very unlikely that either table would have duplicates now. The code around both tables checks for existing values before saving a new one, so only race conditions can create duplicates. Problems with the external vocab table immediately start causing edit failures, so in addition to a race condition being unlikely (parallel changes and ext. vocab enabled), the problem would be immediately noticed. For the oaiset table, it seems even more unlikely that duplicates would exist as the code there also appears to check for an existing value with the same spec and there isn't any parallel case where one would expect multiple oaisets to be being created at once. It isn't clear to me that we can automate removing duplicates if they do exist, especially for oaiset where the descriptions and other fields may differ. My guess is that handling this with a release note instruction might be enough as problems should be extremely rare.

Suggestions on how to test this:

Regression: verify that OAI sets can be created/used and that external vocab values can be added to datasets and their metadata appears in the exported metadata. IN either case it appears to be hard to test the improvement from having the constraints as one would have to run multiple adds of an OAISet/multiple edits to datasets with the same ext. vocab value terms to be able to trigger the problem. (Prior to the PR, parallel changes would add duplicates to the database which you could see there and, at least for ext. vocab, you'd see failures related to multi-value exceptions when trying to retrieve ext. vocab values. After the PR, you shouldn't ever have duplicate values. For OAISets you might get errors when trying to run parallel ops. For the external vocab case, you'd see a fine message in the log about an attempt to write a duplicate value but the overall edit/metadata change would succeed.)

One other thing to test - have a dataset that has uses a type with controlled vocabulary values and then change the definition of the type to not use CVV. Make sure that this does not adversely affect the functionality (display, indexing, etc) of the existing dataset. (adding this because if the change to line 287 of DatasetFieldType.java.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?: included

Additional documentation:

coveralls · 2023-10-06T22:19:10Z

coverage: 20.267% (-0.008%) from 20.275%
when pulling f3fae4b on GlobalDataverseCommunityConsortium:IQSS/9983-missing_unique_constraints
into 4b5cbe8 on IQSS:develop.

…rolledVocabulary instead of looking up complete list of terms to see if it is empty

…straints Amendment

* Applied IQSS#9984 * Merge IQSS#9955 --------- Co-authored-by: Jan van Mansum <[email protected]> Co-authored-by: Jan van Mansum <[email protected]>

Amend metadata block TSVs

…ique_constraints

….com/GlobalDataverseCommunityConsortium/dataverse.git into IQSS/9983-missing_unique_constraints

…ique_constraints

PaulBoon · 2024-01-30T09:28:24Z

@qqmyers Can this be merged into the next release or is there something blocking it, besides the review?

cmbz · 2024-01-30T16:52:15Z

2024/01/30

Prioritized and moved to Sprint Ready after Slack conversation with @qqmyers and @scolapasta

scolapasta

Overall, looks good, had a few basic questions.

scolapasta · 2024-02-05T22:15:29Z

doc/release-notes/9983-unique-constraints.md

+
+
+
+TODO: Add note about reloading metadata blocks after upgrade.


is this a todo that we want before we qa / merge this?

I think that is just a note that the main release notes should include the reload block instructions for these blocks.

Do you guys think we should clarify that whoever makes the release notes has to do this? I am not sure it is clear enough with the TODO: probably we could just mention it on Slack but we run the risk of someone missing the message 🤷🏼 @scolapasta @qqmyers

scolapasta · 2024-02-05T22:17:06Z

scripts/api/data/metadatablocks/astrophysics.tsv

@@ -2,13 +2,13 @@
 	astrophysics		Astronomy and Astrophysics Metadata													
 #datasetField	name	title	description	watermark	 fieldType	displayOrder	displayFormat	advancedSearchField	allowControlledVocabulary	allowmultiples	facetable	displayoncreate	required	parent	metadatablock_id
 	astroType	Type	The nature or genre of the content of the files in the dataset.		text	0		TRUE	TRUE	TRUE	TRUE	FALSE	FALSE		astrophysics
-	astroFacility	Facility	The observatory or facility where the data was obtained. 		text	1		TRUE	TRUE	TRUE	TRUE	FALSE	FALSE		astrophysics


I assume these relate to the todo? What is the reason for changing these values? (I probably just missed it, now I'm just curious)

These are the reason the blocks would need to be reloaded. I think these are inconsistencies - the block says they are controlled vocab fields but there are no vocab values for them in the block. (Which is noticeable once you start looking at the definition rather than for the existence of controlled vocab values to decide what input type to display.)

That makes sense - I just wonder how why they were ever true in the first place?

scolapasta · 2024-02-05T22:19:36Z

src/main/java/edu/harvard/iq/dataverse/DatasetFieldType.java

@@ -284,7 +284,7 @@ public void setDisplayOnCreate(boolean displayOnCreate) {
    }

    public boolean isControlledVocabulary() {
-        return controlledVocabularyValues != null && !controlledVocabularyValues.isEmpty();
+        return allowControlledVocabulary;


is this change because we think the definition is enough (and more important than actual values). Not sure, but could we ever get a case where a field changes FROM allow controlled vocab to disallowing, but we still have old datasets with CVV?

@janvanmansum may know more here but my understanding is that this is primarily being used to decide if you display the controlled vocab list for metadata input or not. So with the change if you set the field to not be cvv, it will now show as a text input, whereas before, if you ever had cvv values in the db, the field shows as cvv going forward. I don't think it affects things if there are specific field values in datasets that have to display, its just about the type.

@qqmyers that makes sense. My concern would be if there are other places that use it for either display or indexing tasks and how this would affect a dataset that had controlled vocabulary values before the type was changed (rare, but could happen. I'll just add a note in the testing section to see if that can be looked at and make sure things look fine.

Looking at code outside the PR- there is an isAllowControlledVocabulary() method above that returns allowControlledVocabulary (so a duplicate now). Perhaps the xhtml or other places where we need this change could just switch to use that method.

FWIW: It does seem to be an odd case to have former CVVs - we're potentially relying on the metadatablock loader to keep old values/ people to keep the old values in the tsv when the field no longer uses them, etc. (I can hardly wait until they make the field CVV again and want new values and not have the old ones show up as choices ...)

I do agree it's an edge case. It's possible any time we would switch from cvv to not, we'd write a db script to convert the values anyway?

scolapasta · 2024-02-08T19:20:33Z

scripts/api/data/metadatablocks/astrophysics.tsv

@@ -2,13 +2,13 @@
 	astrophysics		Astronomy and Astrophysics Metadata													
 #datasetField	name	title	description	watermark	 fieldType	displayOrder	displayFormat	advancedSearchField	allowControlledVocabulary	allowmultiples	facetable	displayoncreate	required	parent	metadatablock_id
 	astroType	Type	The nature or genre of the content of the files in the dataset.		text	0		TRUE	TRUE	TRUE	TRUE	FALSE	FALSE		astrophysics
-	astroFacility	Facility	The observatory or facility where the data was obtained. 		text	1		TRUE	TRUE	TRUE	TRUE	FALSE	FALSE		astrophysics


That makes sense - I just wonder how why they were ever true in the first place?

scolapasta · 2024-02-08T19:24:38Z

src/main/java/edu/harvard/iq/dataverse/DatasetFieldType.java

@@ -284,7 +284,7 @@ public void setDisplayOnCreate(boolean displayOnCreate) {
    }

    public boolean isControlledVocabulary() {
-        return controlledVocabularyValues != null && !controlledVocabularyValues.isEmpty();
+        return allowControlledVocabulary;


@qqmyers that makes sense. My concern would be if there are other places that use it for either display or indexing tasks and how this would affect a dataset that had controlled vocabulary values before the type was changed (rare, but could happen. I'll just add a note in the testing section to see if that can be looked at and make sure things look fine.

…ique_constraints

qqmyers added 3 commits October 6, 2023 17:34

add missing db constriants, add cvoc transaction and try/catch

aae043c

add truncate for vocab table

e29b2b5

add release note, remote truncate

39a2cf5

janvanmansum added a commit to janvanmansum/dataverse that referenced this pull request Oct 10, 2023

Applied IQSS#9984

05b30d5

Changed DatasetFieldType.isControlledVocabulary() to return allowCont…

1c7bc24

…rolledVocabulary instead of looking up complete list of terms to see if it is empty

janvanmansum mentioned this pull request Oct 10, 2023

DD-1442 Ingest-problems Dataverse v5.14 DANS-KNAW/dataverse#179

Merged

Merge pull request #37 from janvanmansum/IQSS/9983-missing_unique_con…

e6ef39c

…straints Amendment

janvanmansum mentioned this pull request Oct 11, 2023

Merge https://github.com/IQSS/dataverse/pull/9955 DANS-KNAW/dataverse#181

Merged

qqmyers changed the title ~~add missing db constriants, add cvoc transaction and try/catch~~ add missing db constraints, add cvoc transaction and try/catch Oct 11, 2023

qqmyers added the Size: 10 A percentage of a sprint. 7 hours. label Oct 11, 2023

janvanmansum added a commit to DANS-KNAW/dataverse that referenced this pull request Oct 12, 2023

Add patch4 to dvnl (#182)

1706fd9

* Applied IQSS#9984 * Merge IQSS#9955 --------- Co-authored-by: Jan van Mansum <[email protected]> Co-authored-by: Jan van Mansum <[email protected]>

qqmyers added the GDCC: DANS related to GDCC work for DANS label Nov 6, 2023

janvanmansum and others added 2 commits November 8, 2023 15:21

Amend metadata block TSVs

43fcf1d

Merge pull request #38 from janvanmansum/amend_controlledVocabsFields

ecb5036

Amend metadata block TSVs

PaulBoon mentioned this pull request Nov 22, 2023

Re-apply PATCH-4 to v6.0 DANS-KNAW/dataverse#191

Merged

qqmyers changed the title ~~add missing db constraints, add cvoc transaction and try/catch~~ External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch Jan 3, 2024

qqmyers added 4 commits January 24, 2024 16:47

Merge remote-tracking branch 'IQSS/develop' into IQSS/9983-missing_un…

b644d95

…ique_constraints

Updating flyway name

93de747

Merge branch 'IQSS/9983-missing_unique_constraints' of https://github…

d76ed26

….com/GlobalDataverseCommunityConsortium/dataverse.git into IQSS/9983-missing_unique_constraints

Merge remote-tracking branch 'IQSS/develop' into IQSS/9983-missing_un…

7849b17

…ique_constraints

scolapasta reviewed Feb 5, 2024

View reviewed changes

scolapasta assigned scolapasta and qqmyers and unassigned qqmyers and scolapasta Feb 5, 2024

scolapasta approved these changes Feb 8, 2024

View reviewed changes

jp-tosca self-assigned this Feb 13, 2024

qqmyers added 3 commits February 13, 2024 13:24

Merge remote-tracking branch 'IQSS/develop' into IQSS/9983-missing_un…

c1111be

…ique_constraints

change flyway numbering

f612f7a

add clarity

f3fae4b

jp-tosca merged commit ba730cf into IQSS:develop Feb 14, 2024
11 checks passed

pdurbin added this to the 6.2 milestone Feb 14, 2024

landreev mentioned this pull request Mar 29, 2024

6.2 release notes #10432

Merged

scolapasta mentioned this pull request Apr 3, 2024

Fields in metadata block TSV files have allowControlledVocabulary property set to true, but no CV terms #8857

Closed

jggautier mentioned this pull request Apr 3, 2024

Feature Request/Idea: Consider error handling when trying to load metadata block where field's allowControlledVocabulary property is TRUE but vocabulary terms are missing #10461

Open

jp-tosca removed their assignment Apr 15, 2024

jggautier mentioned this pull request Jul 19, 2024

Missing constraints allow duplicate values for external CVoc and oaiset #9983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch #9984

External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch #9984

qqmyers commented Oct 6, 2023 •

edited by scolapasta

Loading

coveralls commented Oct 6, 2023 •

edited

Loading

PaulBoon commented Jan 30, 2024

cmbz commented Jan 30, 2024

scolapasta left a comment

scolapasta Feb 5, 2024

qqmyers Feb 5, 2024

jp-tosca Feb 13, 2024

scolapasta Feb 5, 2024

qqmyers Feb 5, 2024 •

edited

Loading

scolapasta Feb 8, 2024

scolapasta Feb 5, 2024

qqmyers Feb 5, 2024

scolapasta Feb 8, 2024

qqmyers Feb 8, 2024

scolapasta Feb 8, 2024

scolapasta Feb 8, 2024

scolapasta Feb 8, 2024




		TODO: Add note about reloading metadata blocks after upgrade.

External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch #9984

External Controlled Vocab: add missing db constraints, add cvoc transaction and try/catch #9984

Conversation

qqmyers commented Oct 6, 2023 • edited by scolapasta Loading

coveralls commented Oct 6, 2023 • edited Loading

PaulBoon commented Jan 30, 2024

cmbz commented Jan 30, 2024

scolapasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqmyers Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqmyers commented Oct 6, 2023 •

edited by scolapasta

Loading

coveralls commented Oct 6, 2023 •

edited

Loading

qqmyers Feb 5, 2024 •

edited

Loading