Improve parsing of short DOIs #6920

PremKolar · 2020-09-17T19:02:19Z

Fixes #6880

no more erroneous interpretation as short DOI of url's/notes like:

"http://www.cs.utexas.edu/users/kaufmann/itp-trusted-extensions-aug-2010/summary/summary.pdf"
"abc.com/20/10/something/whatever"

still correct detection of short DOIs hidden in url's as long as doi is part of subdomain, eg:

"www.doi.xyz.com/10/abc123"

correct detection of short DOIs like:

"doi.org/ab123"
"https://doi.org/ab123"

added respective tests.

added entry to CHANGELOG

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

fix merge

tobiasdiez

Thanks a lot. It's a nice improvement, and I really like that you added many tests for it. The code looks good to me, I've only a few nitpick comments.

For the code styling, have a look at: https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#using-jabrefs-code-style

tobiasdiez · 2020-09-18T07:37:48Z

src/main/java/org/jabref/logic/cleanup/DoiCleanup.java

                for (Field field : FIELDS) {
                    entry.getField(field).flatMap(DOI::parse)
-                         .ifPresent(unused -> removeFieldValue(entry, field, changes));
+                            .ifPresent(unused -> removeFieldValue(entry, field, changes));


please remove the added indent.

tobiasdiez · 2020-09-18T07:40:41Z

src/main/java/org/jabref/model/entry/identifier/DOI.java

+                    this.doi = "10/"+shortcutDoiMatcher.group(1);
+                    isShortDoi = true;
+                } else {
+


please remove empty line

tobiasdiez · 2020-09-18T07:43:12Z

src/main/java/org/jabref/model/entry/identifier/DOI.java

+     * checks whether the DOI might as well be something else, like:
+     * "10/2012" or " 10:as23 ".
+     *
+     * sets the property "isAmbiguous" accordingly. ( NOT USED YET! )


We have the convention that in general dead/unused code should be removed. Do you have any concrete plans/ideas where the isAmbiguous variable is needed?
If not, I would ask you to remove it.

Hi Tobias,
Yes I wasn't sure whether to open a new issue or to discuss my idea here in the comments somewhere:
When dois are cleaned up, the fields Notes and url are scanned for dois and short dois. The way it is implemented now, an entry like eg 10/2021 is interpreted as a short doi, when this might very well be something else, like a date...
checkForAmbiguousShortDOI() detects these ambiguous cases and sets the property isAmbiguous accordingly.
I stopped there, because I am still trying to find out whether a short doi like 10/1234 is even possible i.e. whether it can be fully numeric. I wrote an email to [email protected], but haven't heard back yet...
Also, I wanted to open a discussion on what to do with such ambiguous cases. We could implement some DOIexistanceChecker that checks whether the supposed doi can be found. But a) I am not sure what would be the most efficient way of doing this and b) I am worried about scaling issues when there are thousands of ambiguous dois... Maybe the checking should be optional via a tick-box in respective gui?

That's a good point to be aware of. For the conversation from the Notes and Url fields, I would say we can be optimistic and put it in the doi field if it's look like an DOI. I guess it's very rare to have only a date in the notes field.

I like the idea of checking doi's for existence (as an integrity check). I'm not sure how to implement it effectively, without querying crossref's api for each doi separately.

ok. the way I understand you, the integrity checking would be applied to all DOIs right? should i remove the unused method and property regarding ambiguous short DOIs then?

yes, integrity checks are always applied to all entries; and some users have 10k...

For now I would say remove the code for ambiguous dois. The code is not lost, as we can go back to this PR to copy it once it's really needed.

tobiasdiez · 2020-09-18T07:43:58Z

src/test/java/org/jabref/model/entry/identifier/DOITest.java

        assertEquals("10/gf4gqc", new DOI("doi:10/gf4gqc").getDOI());
    }

+


remove new empty lines

…into bugfix/issue6880

Siedlerchr · 2020-09-18T14:18:06Z

Please fix the checkstyle issues. You can install the extension in Intellij directly:
https://github.com/JabRef/jabref/tree/master/config

> Task :classes
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:53:13: '+' is not followed by whitespace. [WhitespaceAround]

> Task :checkstyleMain
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:92:96: ',' is preceded with whitespace. [NoWhitespaceBefore]
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:139:37: '+' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:139:37: '+' is not preceded with whitespace. [WhitespaceAround]

PremKolar · 2020-09-18T15:02:40Z

Please fix the checkstyle issues. You can install the extension in Intellij directly:
https://github.com/JabRef/jabref/tree/master/config

> Task :classes
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:53:13: '+' is not followed by whitespace. [WhitespaceAround]

> Task :checkstyleMain
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:92:96: ',' is preceded with whitespace. [NoWhitespaceBefore]
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:139:37: '+' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/model/entry/identifier/DOI.java:139:37: '+' is not preceded with whitespace. [WhitespaceAround]

Hi Siedlerchr,
Whenever I try to add the checkstyle file to the checkstyle plugin I get the following error:


com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module TreeWalker - Unable to instantiate 'InvalidJavadocPosition' class, it is also not possible to instantiate it as .InvalidJavadocPosition, InvalidJavadocPositionCheck, .InvalidJavadocPositionCheck. Please recheck that class name is specified as canonical name or read how to configure short name usage https://checkstyle.org/config.html#Packages. Please also recheck that provided ClassLoader to Checker is configured correctly.
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:463)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:198)
	at org.infernus.idea.checkstyle.service.cmd.OpCreateChecker.execute(OpCreateChecker.java:61)
	at org.infernus.idea.checkstyle.service.cmd.OpCreateChecker.execute(OpCreateChecker.java:26)
	at org.infernus.idea.checkstyle.service.CheckstyleActionsImpl.executeCommand(CheckstyleActionsImpl.java:130)
	at org.infernus.idea.checkstyle.service.CheckstyleActionsImpl.createChecker(CheckstyleActionsImpl.java:60)
	at org.infernus.idea.checkstyle.service.CheckstyleActionsImpl.createChecker(CheckstyleActionsImpl.java:51)
	at org.infernus.idea.checkstyle.checker.CheckerFactoryWorker.run(CheckerFactoryWorker.java:46)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to instantiate 'InvalidJavadocPosition' class, it is also not possible to instantiate it as .InvalidJavadocPosition, InvalidJavadocPositionCheck, .InvalidJavadocPositionCheck. Please recheck that class name is specified as canonical name or read how to configure short name usage https://checkstyle.org/config.html#Packages. Please also recheck that provided ClassLoader to Checker is configured correctly.
	at com.puppycrawl.tools.checkstyle.PackageObjectFactory.createModule(PackageObjectFactory.java:210)
	at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:152)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:198)
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:458)
	... 7 more

What am I doing wrong?
Choosing the IntelliJ IDEA code style XML as style file works, but that alone doesn't have any discernible effect on anything... Sorry, I am quite new to Java and very new to IntelliJ IDEA...

Siedlerchr · 2020-09-19T13:17:38Z

No idea what's wrong with the plugin, but you can always execute ./gradlew checkstyle on the comandline

Siedlerchr

Looks good to me now! Thanks for tackling this issue

koppor · 2020-09-20T20:06:57Z

src/test/java/org/jabref/model/entry/identifier/DOITest.java

    @Test
    public void acceptURNPrefix() {
        assertEquals("10.123/456", new DOI("urn:10.123/456").getDOI());
-        assertEquals("10.123/456", new DOI("urn:doi:10.123/456").getDOI());


Note to self: urn:doi is not a valid DOI: See https://en.wikipedia.org/wiki/Digital_object_identifier#Standardization

koppor

LGTM now

…-issue-6707 * upstream/master: (35 commits) Fix a fetcher test for the ShortDOIService (#6945) Fixes Shared Database: Changes filtering in CoarseChangeFilter to attribute property (#6868) Changed default value of "search and store files relative to bibtex file" to true (#6928) Replace comment by just a failure (#6943) Fix: in entry types editor selected field is not removed after first click (#6941) Fix remove actions for entry types in the editor (#6933) Always use Java 15 (#6929) Update DevDocs: workaround for issues with local openjfx maven libraries (#6931) Fixes bugs in the `regex` cite key pattern modifier (#6893) Add missing author Readability for citation key patterns (#6706) Add new author Reset to master and add default case to switch (#6847) Bump mockito-core from 3.5.10 to 3.5.11 (#6924) Bump byte-buddy-parent from 1.10.14 to 1.10.15 (#6923) Bump org.beryx.jlink from 2.21.4 to 2.22.0 (#6925) Bump xmpbox from 2.0.20 to 2.0.21 (#6926) Bump pascalgn/automerge-action from v0.9.0 to v0.10.0 (#6927) Improve parsing of short DOIs (#6920) Bump junit-vintage-engine from 5.6.2 to 5.7.0 (#6910) ...

PremKolar added 2 commits September 17, 2020 20:38

fixed issue JabRef#6880: Improved detection of short DOIs.

68c4beb

updated changelog for issue JabRef#6880

404a41a

Siedlerchr changed the title ~~Bugfix/issue6880~~ Improve parsing of short DOIs Sep 18, 2020

Merge branch 'master' into bugfix/issue6880

d57fbc6

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 18, 2020

Update CHANGELOG.md

d0208f4

fix merge

tobiasdiez requested changes Sep 18, 2020

View reviewed changes

tobiasdiez added the status: changes-required Pull requests that are not yet complete label Sep 18, 2020

PremKolar added 3 commits September 18, 2020 10:42

fixed formatting/style

be8fd19

Merge branch 'bugfix/issue6880' of https://github.com/PremKolar/jabref …

98ea42f

…into bugfix/issue6880

removed stuff relating to 'ambiguous DOIs'

0a21ee2

Merge branch 'master' into bugfix/issue6880

aac71bd

PremKolar added 2 commits September 19, 2020 20:31

fixed checkstyle failures

1e5de79

re-added the 'urn:doi:' case for FIND_SHORT_DOI_EXP

e15cd89

Siedlerchr approved these changes Sep 20, 2020

View reviewed changes

koppor reviewed Sep 20, 2020

View reviewed changes

koppor approved these changes Sep 20, 2020

View reviewed changes

koppor merged commit d944eae into JabRef:master Sep 20, 2020

koppor removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers status: changes-required Pull requests that are not yet complete labels Sep 20, 2020

koobs mentioned this pull request Nov 27, 2020

Additional Short DOI fatal exception cases: java.lang.IllegalArgumentException: <string> is not a valid DOI/Short #7127

Closed

1 task

PremKolar mentioned this pull request Dec 14, 2020

Fix Short DOI error #7191

Merged

5 tasks

		assertEquals("10/gf4gqc", new DOI("doi:10/gf4gqc").getDOI());
		}

Uh oh!

Improve parsing of short DOIs #6920

Improve parsing of short DOIs #6920

Uh oh!

Conversation

PremKolar commented Sep 17, 2020 • edited by Siedlerchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

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!

PremKolar Sep 18, 2020 • 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!

Siedlerchr commented Sep 18, 2020

Uh oh!

PremKolar commented Sep 18, 2020

Uh oh!

Siedlerchr commented Sep 19, 2020

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PremKolar commented Sep 17, 2020 •

edited by Siedlerchr

Loading

PremKolar Sep 18, 2020 •

edited

Loading