Skip to content
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.

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

Improve error message when the keystore/truststore type cannot be detected from the file extension #39164

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

cescoffier
Copy link
Member

The error message was misleading when the type of a keystore or truststore cannot be found from the file extension.
This PR fixes and improves it by indicating the property to configure.

@michalvavrik Your previous report highlighted a problem in the error message. Here is the fix.

@@ -66,7 +70,8 @@ public static TrustOptions computeTrustOptions(CertificateConfig certificates, O
}

if (singleTrustStoreFile != null) { // We have a single trust store file.
String type = certificates.trustStoreFileType.orElse(getTypeFromFileName(singleTrustStoreFile));
String type = certificates.trustStoreFileType.map(String::toLowerCase)
Copy link
Member

Choose a reason for hiding this comment

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

changes are fine, however always calling certificates.trustStoreFileType.map(String::toLowerCase).orElse(getTypeFromFileName("truststore", singleTrustStoreFile)) means that when file extension is not present or when truststore file type can't be matched based on the file extension, an exception is thrown, doesn't it? wouldn't orElseGet be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, you are right!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, in addition, I added a few tests.... because at least we have a baseline.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I didn't use orElseGet, as it introduces a lambda, in this case, the imperative if/then block is more efficient)

Copy link
Member

Choose a reason for hiding this comment

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

Great, thank you.

This comment has been minimized.

@cescoffier cescoffier force-pushed the keystore-error-message branch from 3d1645a to 7904bba Compare March 5, 2024 07:37
Copy link

quarkus-bot bot commented Mar 5, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7904bba.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.


Flaky tests - Develocity

⚙️ Gradle Tests - JDK 17

📦 integration-tests/gradle

io.quarkus.gradle.builder.QuarkusModelBuilderTest.shouldLoadSimpleModuleDevModel - History

  • Could not run build action using connection to Gradle distribution 'https://services.gradle.org/distributions/gradle-8.6-bin.zip'. - org.gradle.tooling.BuildException
org.gradle.tooling.BuildException: Could not run build action using connection to Gradle distribution 'https://services.gradle.org/distributions/gradle-8.6-bin.zip'.
	at org.gradle.tooling.internal.consumer.ExceptionTransformer.transform(ExceptionTransformer.java:51)
	at org.gradle.tooling.internal.consumer.ExceptionTransformer.transform(ExceptionTransformer.java:29)
	at org.gradle.tooling.internal.consumer.ResultHandlerAdapter.onFailure(ResultHandlerAdapter.java:43)
	at org.gradle.tooling.internal.consumer.async.DefaultAsyncConsumerActionExecutor$1$1.run(DefaultAsyncConsumerActionExecutor.java:69)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
	at org.gradle.internal.concurrent.AbstractManagedExecutor$1.run(AbstractManagedExecutor.java:47)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)

io.quarkus.gradle.builder.QuarkusModelBuilderTest.shouldLoadSimpleModuleTestModel - History

  • Could not run build action using connection to Gradle distribution 'https://services.gradle.org/distributions/gradle-8.6-bin.zip'. - org.gradle.tooling.BuildException
org.gradle.tooling.BuildException: Could not run build action using connection to Gradle distribution 'https://services.gradle.org/distributions/gradle-8.6-bin.zip'.
	at org.gradle.tooling.internal.consumer.ExceptionTransformer.transform(ExceptionTransformer.java:51)
	at org.gradle.tooling.internal.consumer.ExceptionTransformer.transform(ExceptionTransformer.java:29)
	at org.gradle.tooling.internal.consumer.ResultHandlerAdapter.onFailure(ResultHandlerAdapter.java:43)
	at org.gradle.tooling.internal.consumer.async.DefaultAsyncConsumerActionExecutor$1$1.run(DefaultAsyncConsumerActionExecutor.java:69)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
	at org.gradle.internal.concurrent.AbstractManagedExecutor$1.run(AbstractManagedExecutor.java:47)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
  • Expecting actual not to be null - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual not to be null
	at io.quarkus.gradle.builder.QuarkusModelBuilderTest.shouldLoadSimpleModuleTestModel(QuarkusModelBuilderTest.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@michalvavrik michalvavrik requested a review from sberyozkin March 5, 2024 11:52
@sberyozkin sberyozkin merged commit 74df926 into quarkusio:main Mar 5, 2024
49 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Mar 5, 2024
@cescoffier cescoffier deleted the keystore-error-message branch March 7, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants