Switch to stream-based loading by koppor · Pull Request #11479 · JabRef/jabref

koppor · 2024-07-10T14:50:38Z

Uses ProtectedTermsLoader.class.getResourceAsStream instead of ProtectedTermsLoader.class.getResource.

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Siedlerchr · 2024-07-10T16:51:01Z

            LOGGER.warn("Could not read terms from file {}", path);
            return;
        }
-        try (Stream<String> lines = Files.lines(path, StandardCharsets.UTF_8)) {


Files lines does not work with graal? Tbh this looks like a major drawback here.

BufferedReader#lines seems to be somewhat of an equivalent here...

In line 60, I use exactly this function.

This call is only a wrapper for the call if a user supplies a file!

GraalVM does not support .class.getResource( with hacks. See oracle/graal#7682 for a longer discussion. Especially oracle/graal#7682 (comment)

The reason NativeImageFileSystem behaves like JarFileSystem, not like UnixFileSystem, is that, unlike UnixFileSystem, you can't, for example, delete a file (resource) on both NativeImageFileSystem and JarFileSystem. The resources in NativeImageFileSystem are baked into the read-only section of the image heap

koppor · 2024-07-10T22:02:26Z

... and the tests fail... OMG

Test readProtectedTermsListFromFileReadsDescription() FAILED

Siedlerchr · 2024-07-10T22:10:17Z

-            LOGGER.error("");
+        description = descriptionString;
+        location = resourceFileName;
+        try (InputStream inputStream = ProtectedTermsLoader.class.getResourceAsStream(Objects.requireNonNull(resourceFileName))) {


You need to use getClassLoader() first, don't ask me why but for getResourceAsStream it does not work otherwise

Siedlerchr · 2024-07-10T22:11:15Z

-            LOGGER.error("");
+        description = descriptionString;
+        location = resourceFileName;
+        try (InputStream inputStream = ProtectedTermsLoader.class.getResourceAsStream(Objects.requireNonNull(resourceFileName))) {


Suggested change

try (InputStream inputStream = ProtectedTermsLoader.class.getResourceAsStream(Objects.requireNonNull(resourceFileName))) {

try (InputStream inputStream = ProtectedTermsLoader.class.getClassLoader().getResourceAsStream(Objects.requireNonNull(resourceFileName))) {

Works without here.

But might not work at runtime

I tried with ./gradlew run and then protected terms. Worked.

koppor · 2024-07-11T12:14:39Z

For full GraalVM, we would maybe to patch CSL.java:

            // try to find style in classpath
            url = CSL.class.getResource(styleName);

However, I added a workaround, because this branch is only taken if the provided String is not an XML file.

koppor · 2024-07-11T12:25:55Z

Assuming that JavaFX can perfectly deal with URLs coming from the classpath, we have one class left to "really" change:

org.jabref.logic.citationstyle.CitationStyle#discoverCitationStyles: Here, the list could be generated during compilation time

koppor · 2024-07-12T08:01:54Z


            // No SystemId required, because no relative URLs need to be resolved
-            Source xsltSource = new StreamSource(xsltResource);
+            Source xsltSource = new StreamSource(xsltResource, MathMLParser.class.getResource(XSLT_FILE_PATH).toExternalForm());


OK for a workaround. - Can we do add a fake path here? Maybe, #future work

koppor · 2024-07-12T08:08:11Z

Proposal: Get this in to see whether there are issues. As follow up the "difficult" conversions.

Siedlerchr · 2024-07-15T20:11:41Z

Use the binary to test it!

github-actions · 2024-07-15T20:28:19Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

koppor · 2024-07-16T11:34:26Z

Tested. Protected terms works. Think, more users should test it and we can revert if needed ^^.

calixtus

Lets move forward here.

* upstream/main: Fix NPE when saving preferences (JabRef#11509) Switch to stream-based loading (JabRef#11479) Save unlinked local files dialog prefs (JabRef#11493) Add minimal support for biblatex data annotations (JabRef#11506) Fix handling of relative-file storage and auto linking (JabRef#11492) New Crowdin updates (JabRef#11504) Add missing issue numbers CSL4LibreOffice - A [GSoC '24] (JabRef#11477) Bump src/main/resources/csl-styles from `b2be5ae` to `fd6cb3e` (JabRef#11501) Bump gittools/actions from 1.1.1 to 1.2.0 (JabRef#11500) Bump com.kohlschutter.junixsocket:junixsocket-core from 2.9.1 to 2.10.0 (JabRef#11498) Bump commons-logging:commons-logging from 1.3.2 to 1.3.3 (JabRef#11499) Bump org.jsoup:jsoup from 1.17.2 to 1.18.1 (JabRef#11497) Bump com.kohlschutter.junixsocket:junixsocket-mysql from 2.9.1 to 2.10.0 (JabRef#11496) Bump org.openrewrite.recipe:rewrite-recipe-bom from 2.14.0 to 2.15.0 (JabRef#11495) FAQ updates (JabRef#11486) Update Gradle Wrapper from 8.8 to 8.9. Fix Chocolate.bib (JabRef#11491) # Conflicts: # src/main/java/org/jabref/gui/openoffice/OOBibBase.java # src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java # src/main/java/org/jabref/gui/openoffice/StyleSelectDialogView.java # src/main/java/org/jabref/gui/openoffice/StyleSelectDialogViewModel.java # src/main/java/org/jabref/preferences/JabRefPreferences.java

Switch to stream-based loading

2655432

koppor added this to the 6.0-alpha milestone Jul 10, 2024

Fix typos

157a448

Siedlerchr reviewed Jul 10, 2024

View reviewed changes

calixtus and others added 6 commits July 11, 2024 08:41

Add architecture test

cad404c

Fix ProtectedTermsLoaderTest and minor refactor

5dbc4a3

Remove "static"

11c1be7

Introduce AllowedToUseClassGetResource - and do some rewrite

aa8f549

Streamline FileUtil.copy

ba104cb

Rewrite CitationStyle.java more to streams

c140774

github-actions Bot reviewed Jul 11, 2024

View reviewed changes

Comment thread src/main/java/org/jabref/logic/citationstyle/CitationStyle.java Outdated

Comment thread src/main/java/org/jabref/logic/citationstyle/CitationStyle.java Outdated

More change to asStream

66d2d64

github-actions Bot reviewed Jul 11, 2024

View reviewed changes

Comment thread src/main/java/org/jabref/logic/net/ssl/TrustStoreManager.java Outdated

Convert to getResourceAsStream

338f7b0

koppor and others added 3 commits July 11, 2024 14:27

Remoe unused imports

17aa7c0

Merge branch 'main' into switch-to-stream

acee78e

Fix MathMLParser

4e3a407

koppor commented Jul 12, 2024

View reviewed changes

koppor added 2 commits July 12, 2024 10:04

Remove imports

599b980

Fix test

f36dfa3

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 12, 2024

koppor enabled auto-merge July 15, 2024 20:10

Merge branch 'main' into switch-to-stream

0f068f3

Siedlerchr disabled auto-merge July 15, 2024 20:11

Siedlerchr enabled auto-merge July 15, 2024 20:28

calixtus approved these changes Jul 17, 2024

View reviewed changes

Siedlerchr added this pull request to the merge queue Jul 17, 2024

calixtus added the dev: code-quality Issues related to code or architecture decisions label Jul 17, 2024

Merged via the queue into main with commit 4a8404c Jul 17, 2024

Siedlerchr deleted the switch-to-stream branch July 17, 2024 18:36

	try (InputStream inputStream = ProtectedTermsLoader.class.getResourceAsStream(Objects.requireNonNull(resourceFileName))) {
	try (InputStream inputStream = ProtectedTermsLoader.class.getClassLoader().getResourceAsStream(Objects.requireNonNull(resourceFileName))) {

Uh oh!

Conversation

koppor commented Jul 10, 2024

Mandatory checks

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!

koppor commented Jul 10, 2024

Uh oh!

Siedlerchr Jul 10, 2024 • 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!

koppor commented Jul 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koppor commented Jul 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Jul 12, 2024

Uh oh!

Siedlerchr commented Jul 15, 2024

Uh oh!

github-actions Bot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koppor commented Jul 16, 2024

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Siedlerchr Jul 10, 2024 •

edited

Loading

github-actions Bot commented Jul 15, 2024 •

edited

Loading