feat: migrate personOrOrg settings to MPConfig - #11485
Conversation
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
Thanks for the pull request! I left some comments.
| The Schema.org metadata and OpenAIRE exports and the Schema.org metadata included in DatasetPages try to infer whether each entry in the various fields (e.g. Author, Contributor) is a Person or Organization. If you are sure that | ||
| users are following the guidance to add people in the recommended family name, given name order, with a comma, you can set this true to always assume entries without a comma are for Organizations. The default is false. | ||
|
|
||
| Can also be set via *MicroProfile Config API* sources, e.g. the environment variable ``DATAVERSE_PERSONORORG_ASSUMECOMMAINPERSONNAME``. |
There was a problem hiding this comment.
Hmm, ASSUMECOMMAINPERSONNAME is a bit hard to read. I understand you're keeping it the same as the old setting: dataverse.personOrOrg.assumeCommaInPersonName.
What sort of options do we have? Can we add underscores like ASSUME_COMMA_IN_PERSON_NAME and still have backward compatibility? (I'm not sure if there's backward compatibility with the ASSUMECOMMAINPERSONNAME anyway.)
Also, can we please have a release note snippet?
There was a problem hiding this comment.
I've now changed the names of the two settings to kebab case, which is what they should be called according to the naming conventions (https://guides.dataverse.org/en/latest/developers/configuration.html#adding-a-jvm-setting).
This means they can now be set using DATAVERSE_PERSON_OR_ORG_ASSUME_COMMA_IN_PERSON_NAME and DATAVERSE_PERSON_OR_ORG_ORG_PHRASE_ARRAY. (Or asadmin create-jvm-options '-Ddataverse.person-or-org.assume-comma-in-person-name=true and asadmin create-jvm-options '-Ddataverse.person-or-org.org-phrase-array=Org,GmbH'). I've tested to confirm all of these options work.
For backwards compatibility, adding an alias so that the old name asadmin create-jvm-options '-Ddataverse.personOrOrg.assumeCommaInPersonName=true' can still be used was easy.
However, for the orgPhraseArray setting, the expected value format has changed as well. Previously, a list like ["Org","GmbH"] was expected, but now it's a list like Org,GmbH. Afaik it wouldn't be as easy to still support parsing the old value format. So I haven't added an alias for that option.
Is it OK to drop support for the old orgPhraseArray setting and require admins to migrate option name + value format? Considering it's documented as an experimental setting.
Also, I've just added a release note snippet that documents all this as well.
pdurbin
added
Component: Containers
ofahimIQSS
moved this from Ready for Triage
to Ready for Review ⏩
in IQSS Dataverse Project
cmbz
added
FY25 Sprint 23
qqmyers
left a comment
qqmyers
left a comment
There was a problem hiding this comment.
Overall, switching to MPConfig this way looks fine. I do wonder whether removing the static initialization is worth it.
| CSL_COMMON_STYLES(SCOPE_CSL, "common-styles"), | ||
|
|
||
| // PersonOrOrgUtil SETTINGS | ||
| SCOPE_PERSONORORG(PREFIX, "person-or-org", "dataverse.personOrOrg"), |
There was a problem hiding this comment.
Mostly curious - is the alias here needed?
There was a problem hiding this comment.
Hm, I am not sure but since I don't see any other scopes with aliases in that file, I guess not. It could be removed.
|
|
||
| boolean isOrganization = !isPerson && Organizations.getInstance().isOrganization(name); | ||
| if (!isOrganization) { | ||
| String[] orgPhrases = JvmSettings.ORG_PHRASE_ARRAY.lookupOptional(String[].class).orElse(new String[]{}); |
There was a problem hiding this comment.
It appears that the shift from a static block to looking these up as needed is just to be able to use the @JvmSetting annotation in tests? Putting the code here means these lookups will be done ~1M times for a /reExportAll command for Harvard Dataverse. Could/should we just call updated versions of the static methods in the tests?
There was a problem hiding this comment.
We discussed this PR today and we're still concerted about these ~1 million calls.
@landreev is on vacation for a week or so but we'd like him to take a look when he gets back.
There was a problem hiding this comment.
I agree that we should avoid making these lookups in normal operations, if at all possible. Sorry I missed this.
There was a problem hiding this comment.
@vera - can you revert the change here that and go back to having a static initialization and setX methods for use in testing? I'll move this to In Progress until you're finished.
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
|
it looks like readthedocs job failed |
|
Here's the failure: Warning, treated as error: |
|
I fixed the Sphinx errors in 9a5a102. |
|
@landreev Jim and I would like you to take a look, please. This might result in a lot of calls on export. |
|
@vera can you please resolve merge conflicts? |
# Conflicts: # src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java
Yes, done! |
qqmyers
left a comment
qqmyers
left a comment
There was a problem hiding this comment.
Looks good - thanks for reverting the dynamic lookup.
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
|
tests passed - merging PR |
github-project-automation
Bot
moved this from QA ✅
to Merged 🚀
in IQSS Dataverse Project
8 participants
What this PR does / why we need it:
As discussed on Zulip (https://dataverse.zulipchat.com/#narrow/channel/375707-community/topic/Setting.20dataverse.2Efiles.2Ehide-schema-dot-org-download-urls/with/514143249) for a different config option, this PR also migrates the options
dataverse.personOrOrg.orgPhraseArrayanddataverse.personOrOrg.assumeCommaInPersonNameto MPConfig.Which issue(s) this PR closes:
Special notes for your reviewer:
/
Suggestions on how to test this:
Running the tests:
mvn test -Dtest="PersonOrOrgUtilTest"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?:
I think that this PR contains a non-backwards-compatible change, with the expected format of the orgPhraseArray changing from a JSON array (e.g.
["Portable","GmbH"]) to a comma-separated list of values (Portable,GmbH), and a release note should be added for that, unless we mitigate this in some other way. Please let me know what you think about this, because I am unsure.Additional documentation:
/