Skip to content

chore: bump-okhttp-4.12.0-to-5.0.0 #13521

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 11, 2025
Merged

chore: bump-okhttp-4.12.0-to-5.0.0 #13521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a70d8b
chore: bump-okhttp-4.12.0-to-5.0.0
Munhangyeol Jul 10, 2025
aace5c7
Discard changes to jabkit/src/test/java/org/jabref/cli/ArgumentProces…
koppor Jul 10, 2025
5e07ec7
Update build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-…
Munhangyeol Jul 10, 2025
3d32f6e
Merge branch 'main' into chore/bump-okhttp-4.12.0-to-5.0.0
koppor Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ extraJavaModuleInfo {
}
module("com.konghq:unirest-modules-gson", "unirest.modules.gson")
module("com.squareup.okhttp3:okhttp", "okhttp3")
module("com.squareup.okhttp3:okhttp-sse", "okhttp3.sse")

module("com.squareup.okhttp3:okhttp-jvm", "okhttp3")
Copy link
Member

@koppor koppor Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, now two modules have the same name - see line 149.

Copy link
Contributor Author

@Munhangyeol Munhangyeol Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern. Even though two modules are assigned the same name here, there is no actual conflict because they refer to completely separate artifacts (okhttp vs. okhttp-jvm).

This duplication is currently necessary because some dependencies (such as langchain4j-hugging-face) still require the legacy okhttp artifact, while our project relies on the newer okhttp-jvm from OkHttp 5.x.

Since the Java Module System (JPMS) treats them as distinct jars, and as long as both modules are not loaded into the same named module path, this setup works without any issues.

Copy link
Contributor Author

@Munhangyeol Munhangyeol Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's anything wrong or anything I should fix in what I said, please feel free to let me know :)

Copy link
Member

@koppor koppor Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling says that overloaded module names will cause issues. Especially when we will switch to use modules as the primary source of dependencies: https://github.com/gradlex-org/java-module-dependencies

Copy link
Contributor Author

@Munhangyeol Munhangyeol Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overloaded module names could definitely cause issues, especially if we move towards using modules as the primary way of managing dependencies in the future.
I’ll try to look into this and hopefully resolve it by tomorrow or the day after. Please feel free to let me know if there’s anything I missed or should improve!

Copy link
Contributor Author

@Munhangyeol Munhangyeol Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the dependency tree with the dependencies task, and you're right.
Gradle resolves okhttp to version 5.0.0 automatically because of other dependencies or version conflicts.
However, since langchain4j-hugging-face still relies on the 4.x API, upgrading to 5.0.0 breaks compatibility.

Copy link
Member

@koppor koppor Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a separte module name help to avoid module name overloading?

The modules will be on the same path - as JabRef needs all of them.

Copy link
Contributor Author

@Munhangyeol Munhangyeol Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current situation, module name conflicts are unavoidable, and using separate module names alone does not fundamentally resolve the issue.

Copy link
Collaborator

@jjohannes jjohannes Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

com.squareup.okhttp3:okhttp is not really a module anymore. The Jar is more or less empty now. This is Kotlin multi-platform stuff.

I think it does not end up the Module Path, but is needed by the mechanism that looks at the metadata to create the missing module-info files. That the checks are passing on this PR indicates that everything is ok.

I know that it looks like the name is used twice, but in practice only one of the Jars is used. It's confusing but its Kotlin Multi-Platform libraries "clashing" with JPMS. Maybe add a comment about this to the module("com.squareup.okhttp3:okhttp", "okhttp3") line.

We have a similar situation here with kotlinpoet:

https://github.com/hiero-ledger/hiero-gradle-conventions/blob/main/src/main/kotlin/org.hiero.gradle.base.jpms-modules.gradle.kts#L156-L157

Copy link
Collaborator

@jjohannes jjohannes Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name okhttp3 is correct because the MANIFEST of com.squareup.okhttp3:okhttp-jvm contains

Automatic-Module-Name: okhttp3

module("com.squareup.okhttp3:okhttp-jvm-sse", "okhttp3.sse")
module("com.squareup.okio:okio", "okio")
module("com.squareup.retrofit2:converter-jackson", "retrofit2.converter.jackson")
module("com.squareup.retrofit2:retrofit", "retrofit2")
Expand Down
2 changes: 2 additions & 0 deletions jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ void convertBibtexToTableRefsAsBib(@TempDir Path tempDir) throws URISyntaxExcept

@Test
void checkConsistency() throws URISyntaxException {

Path testBib = Path.of(Objects.requireNonNull(ArgumentProcessorTest.class.getResource("origin.bib")).toURI());
String testBibFile = testBib.toAbsolutePath().toString();

Expand All @@ -146,6 +147,7 @@ void checkConsistency() throws URISyntaxException {
int executionResult = commandLine.execute(args.toArray(String[]::new));

String output = outContent.toString();

assertTrue(output.contains("Checking consistency for entry type 1 of 1\n"));
assertTrue(output.contains("Consistency check completed"));
assertEquals(0, executionResult);
Expand Down
2 changes: 1 addition & 1 deletion versions/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ dependencies.constraints {
api("com.konghq:unirest-java-core:4.4.7")
api("com.konghq:unirest-modules-gson:4.4.7")
api("com.pixelduke:fxthemes:1.6.0")
api("com.squareup.okhttp3:okhttp:4.12.0")
api("com.squareup.okhttp3:okhttp:5.0.0")
api("com.squareup.okio:okio-jvm:3.12.0")
api("com.squareup.retrofit2:retrofit:3.0.0")
api("com.tngtech.archunit:archunit:1.4.1")
Expand Down