Fix conditions that always evaluate to false #5188

gwenneg · 2019-11-04T21:44:00Z

No description provided.

gwenneg · 2019-11-04T21:45:40Z

devtools/maven/src/main/java/io/quarkus/maven/AddExtensionMojo.java

            }
-        } finally {


@aloubyansky I need you to take a look at this one (hide whitespace changes for a much smaller diff). Right now, removing this code doesn't change the plugin behaviour, but I'm pretty sure that's not the way we'd want to fix this class.

Yes, it's not about just fixing this class, it's about fixing the tools. I think it's ok to merge it. The buildFile is closed in AddExtensions.

gwenneg · 2019-11-04T21:46:27Z

...ions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/QuarkusErrorHandler.java

@@ -46,10 +46,10 @@ public void handle(RoutingContext event) {
        String stack = "";
        Throwable exception = event.failure();
        if (showStack && exception != null) {
-            details = generateHeaderMessage(exception, uuid == null ? null : uuid.toString());


See line 44: String uuid = BASE_ID + ERROR_COUNT.incrementAndGet();

gwenneg · 2019-11-04T21:46:52Z

core/deployment/src/main/java/io/quarkus/deployment/recording/BytecodeRecorderImpl.java

@@ -256,8 +256,6 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
                        return null;
                    }
                    ProxyInstance instance = getProxyInstance(returnType);
-                    if (instance == null)
-                        return null;


getProxyInstance(returnType); never returns null.

I'm not sold on this one.

Unless I'm missing something, the only value that can be returned from getProxyInstance(returnType); comes from https://github.com/quarkusio/quarkus/blob/master/core/deployment/src/main/java/io/quarkus/deployment/recording/BytecodeRecorderImpl.java#L312-L313

It's not about missing something. It's just that doesn't cost anything to have it there so I would keep it.

It's back with curly braces in bonus!

gastaldi · 2019-11-05T03:19:09Z

devtools/maven/src/main/java/io/quarkus/maven/AddExtensionMojo.java

            } catch (IOException e) {
-                throw new MojoExecutionException("Unable to update the pom.xml file", e);
+                throw new MojoExecutionException("Failed initialize project configuration tools", e);


Suggested change

throw new MojoExecutionException("Failed initialize project configuration tools", e);

throw new MojoExecutionException("Failed to initialize the project's build descriptor", e);

Changed, thanks for the suggestion!

gsmet

I added a few comments.

gsmet · 2019-11-05T08:20:01Z

integration-tests/vault-app/src/main/java/io/quarkus/it/vault/VaultTestService.java

@@ -39,7 +39,7 @@ public String test() {
            return "someSecret=" + someSecret + "; expected: " + expectedPassword;
        }
        String password = ConfigProviderResolver.instance().getConfig().getValue("password", String.class);
-        if (!expectedPassword.equals(someSecret)) {
+        if (!password.equals(someSecret)) {


This one makes sense but @vsevel could you have a look?

the second case should be between password and expectedPassword (not between password and someSecret) - although checking password against someSecret was already fixing the issue because we checked just before that someSecret is equal to expectedPassword

password and someSecret are calculated, expectedPassword is the reference (could be a constant)
so something like this is more appropriate:

String password = ConfigProviderResolver.instance().getConfig().getValue("password", String.class); if (!expectedPassword.equals(password)) { return "password=" + password + "; expected: " + expectedPassword; }

thanks for catching that one.

Thanks @vsevel! I changed the code according to your suggestion.

gsmet · 2019-11-05T08:20:12Z

...ions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/QuarkusErrorHandler.java

            stack = generateStackTrace(exception);

-        } else if (uuid != null) {
+        } else {


OK for these two.

gsmet · 2019-11-05T08:21:05Z

core/deployment/src/main/java/io/quarkus/deployment/recording/BytecodeRecorderImpl.java

@@ -256,8 +256,6 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
                        return null;
                    }
                    ProxyInstance instance = getProxyInstance(returnType);
-                    if (instance == null)
-                        return null;


I'm not sold on this one.

gwenneg · 2019-11-05T12:55:09Z

@gsmet It should be all good now.

gwenneg · 2019-11-08T06:57:57Z

@gastaldi Could you please do the approving review for this PR? I was waiting for @gsmet approval, but he's away for a few days and all changes have already been approved individually, so it shouldn't be a problem.

gastaldi

LGTM

gwenneg · 2019-11-08T07:22:42Z

Thanks!

gwenneg commented Nov 4, 2019

View reviewed changes

gwenneg requested a review from aloubyansky November 4, 2019 21:52

gastaldi reviewed Nov 5, 2019

View reviewed changes

gwenneg force-pushed the fix-always-false-conditions branch 2 times, most recently from 74ed236 to aa0680a Compare November 5, 2019 07:05

gsmet reviewed Nov 5, 2019

View reviewed changes

Fix conditions that always evaluate to false

90f0001

gwenneg force-pushed the fix-always-false-conditions branch from aa0680a to 90f0001 Compare November 5, 2019 12:51

gastaldi approved these changes Nov 8, 2019

View reviewed changes

gastaldi added this to the 1.1.0 milestone Nov 8, 2019

gastaldi merged commit 5fea8c0 into quarkusio:master Nov 8, 2019

gwenneg deleted the fix-always-false-conditions branch November 8, 2019 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix conditions that always evaluate to false #5188

Fix conditions that always evaluate to false #5188

gwenneg commented Nov 4, 2019

gwenneg Nov 4, 2019 •

edited

Loading

aloubyansky Nov 4, 2019

gwenneg Nov 4, 2019

gwenneg Nov 4, 2019

gsmet Nov 5, 2019

gwenneg Nov 5, 2019

gsmet Nov 5, 2019

gwenneg Nov 5, 2019

gastaldi Nov 5, 2019 •

edited

Loading

gwenneg Nov 5, 2019

gsmet left a comment

gsmet Nov 5, 2019

vsevel Nov 5, 2019 •

edited

Loading

gwenneg Nov 5, 2019

gsmet Nov 5, 2019

gsmet Nov 5, 2019

gwenneg commented Nov 5, 2019

gwenneg commented Nov 8, 2019

gastaldi left a comment

gwenneg commented Nov 8, 2019

	throw new MojoExecutionException("Failed initialize project configuration tools", e);
	throw new MojoExecutionException("Failed to initialize the project's build descriptor", e);

Fix conditions that always evaluate to false #5188

Fix conditions that always evaluate to false #5188

Conversation

gwenneg commented Nov 4, 2019

gwenneg Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gastaldi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsevel Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwenneg commented Nov 5, 2019

gwenneg commented Nov 8, 2019

gastaldi left a comment

Choose a reason for hiding this comment

gwenneg commented Nov 8, 2019

gwenneg Nov 4, 2019 •

edited

Loading

gastaldi Nov 5, 2019 •

edited

Loading

vsevel Nov 5, 2019 •

edited

Loading