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

Jump to bottom

Prevent NullPointerException if any of the signing properties is null but signing isn't required #2268

Merged

bmuschko merged 8 commits into gradle:master from colindean:colindean/check-null-signing-properties-gh2267

Aug 1, 2017

Contributor

colindean commented Jun 9, 2017

If a property such as signing.keyId was set to a null value, perhaps by setting it to the return of System.getenv('KEY_ID') when KEY_ID is unset, the PgpSignatoryFactory would throw an NPE when reading those properties while constructing the PgpSignatory.

This change adds a null check that also bypasses signatory creation when required is false.

Fixes #2267

colindean force-pushed the colindean/check-null-signing-properties-gh2267 branch from 74ab1ca to 4d40f11 Compare

June 9, 2017 01:48

eriwen added the from:contributor label

Contributor

eriwen commented Jul 5, 2017

Hi @colindean. Thanks for the PR. Could you please confirm whether you've signed a Gradle CLA?

eriwen added cla:no in:signing-plugin labels

Contributor Author

colindean commented Jul 5, 2017

I previously signed one with a previous employer, but I've just now signed it for myself, as whom this contribution was made.

eriwen added cla:yes and removed cla:no labels

bmuschko suggested changes

View reviewed changes

Contributor

bmuschko left a comment •

edited

Thanks for providing the pull request. Could you please address the review comments.

Can you please also add an integration test that demonstrates the change of behavior.
Maybe also add a note to the user guide that null values for the properties will throw an exception.

...ects/signing/src/main/java/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactory.java Outdated

    
                              if(prop != null){

                                  values.add(prop);

                              } else {

                                  if(required){

Contributor

bmuschko Jul 28, 2017

This logic is duplicated and should be reused. The else block below it uses the same logic.

Contributor Author

colindean Jul 29, 2017 •

edited

The exception messages differ. Another concern was keeping the C/M/L on the exceptions thrown therein as close to the source of the problem as possible. If I extract this to a method like this:

private boolean throwIfRequired(boolean required, String message)

then I'll have to check the return value of the method in order to know if I should return early from the readProperties method.

I'm drawing a blank presently for ways to accomplish this more cleanly in Java.

Contributor Author

colindean Jul 29, 2017

I made an attempt at refactoring in 1019328. It reads clearer but exposes some sentiment I captured in the commit message: this capability to retrieve a property safely probably belongs to Project as an overloaded method.

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
              class PgpSignatoryFactorySpec extends Specification {

                  //https://github.com/gradle/gradle/issues/2267

                  protected Project aProjectWithNullProperties(){

Contributor

bmuschko Jul 28, 2017

The method can just be static and without protected.

Contributor Author

colindean Jul 29, 2017

✅ in 13032f2

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
              class PgpSignatoryFactorySpec extends Specification {

                  //https://github.com/gradle/gradle/issues/2267

Contributor

bmuschko Jul 28, 2017

Should use @Issue annotation to indicate the issue URL. The annotation should be added on the class level.

Contributor Author

colindean Jul 29, 2017

✅ in 9c421c7

Contributor

bmuschko Jul 31, 2017

Thanks for the pointer. I wasn't aware that we still had other tests with the Spec suffix. We'll likely migrate them to Test at a later time.

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
              import org.gradle.testfixtures.ProjectBuilder

              import spock.lang.*

              class PgpSignatoryFactorySpec extends Specification {

Contributor

bmuschko Jul 28, 2017

In the code base we mainly follow the convention Test for test class names instead of Spec.

Contributor Author

colindean Jul 29, 2017

org.gradle.plugins.signing.signatory.pgp.PgpKeyIdSpec was my reference for creating the test.

✅ in 346af98

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
                          Exception ex = thrown()

                          ex.class != NullPointerException

                  }

                  def "null properties throw a nice exception"() {

Contributor

bmuschko Jul 28, 2017

We should test this for each of the properties. At the moment it will always fail on the first property. Let's use a where clause here + @Unroll. Let's also change "nice" into "human-readable" or "descriptive".

Contributor Author

colindean Jul 29, 2017

Descriptive ✅ in ece23b8

Where clause with @Unroll. I'll have to research how to do what you're asking.

Contributor

bmuschko Jul 31, 2017

OK, you'll find plenty of examples in the Gradle core code base.

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
                      when:

                          factory.createSignatory(project, true)

                      then:

                          thrown InvalidUserDataException

Contributor

bmuschko Jul 28, 2017

Should verify the exception message.

Contributor Author

colindean Jul 29, 2017

✅ in 4eba1fb

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactorySpec.groovy Outdated

    
                      when:

                          factory.createSignatory(project, true)

                      then:

                          //notThrown NullPointerException // does not allow the real exception through

Contributor

bmuschko Jul 28, 2017

This should rather be noExceptionThrown().

Contributor Author

colindean Jul 29, 2017

It's OK if an exception is thrown here, but we want it not to be an NPE. Wouldn't noExceptionThrown() cause all exceptions to fail the test?

bmuschko added the waiting-for:contributor label

bmuschko self-assigned this

colindean added 6 commits

July 28, 2017 20:49


          Adds a check for null values in signing properties

0318c88

If a property such as `signing.keyId` was set to a null value, perhaps by
setting it to the return of `System.getenv('KEY_ID')` when `KEY_ID` is unset,
the PgpSignatoryFactory would throw an NPE when reading those properties while
constructing the PgpSignatory.

This change adds a null check that also bypasses signatory creation when
`required` is false.

Fixes gradle#2267


          Conforms PgpSignatoryFactory unit test to convention

346af98

Requested in https://github.com/gradle/gradle/pull/2268/files#r130180320


          Moves issue URL to @issue annotation

9c421c7


          Use static instead of protected for factory method

13032f2


          Be descriptive, not nice, when checking exception messages

ece23b8

From https://github.com/gradle/gradle/pull/2268/files#r130180748


          Check the exception message contents instead of just the class

4eba1fb

From https://github.com/gradle/gradle/pull/2268/files#r130180785

colindean force-pushed the colindean/check-null-signing-properties-gh2267 branch from 4d40f11 to 4eba1fb Compare

July 29, 2017 00:50


          Refactors safe retrieval of properties from a project

I feel like this should the responsibility of Project. A consumer should be
able to require that a property be present and not null and get exceptions
when the conditions are unmet.

From https://github.com/gradle/gradle/pull/2268/files#r130179732

Contributor Author

colindean commented Jul 29, 2017 •

edited

Can you please also add an integration test that demonstrates the change of behavior.

Can you recommend an example integration test that I can use as inspiration for the right way to do it?

Maybe also add a note to the user guide that null values for the properties will throw an exception.

~~In what repo should I propose this change? I don't see the text of the linked guide anywhere in this repo.~~

Nevermind, my search string was too long for what was in subprojects/docs/src/docs/userguide/signingPlugin.xml literally. ✅ in f45d2bb.

TODO:

Integration test
Where clause with @Unroll in unit test


          Documents exception for null values in signing plugin properties

f45d2bb

From gradle#2268 (review)

Contributor

bmuschko commented Jul 31, 2017

Can you recommend an example integration test that I can use as inspiration for the right way to do it?

Have a look at SigningIntegrationSpec which extends AbstractIntegrationSpec. In this case you'll likely want to go with SigningIntegrationSpec as it already sets up some common infrastructure.

bmuschko suggested changes

View reviewed changes

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactoryTest.groovy

+                      when:
+                          factory.createSignatory(project, true)
+                      then:
+                          Exception ex = thrown()

Contributor

bmuschko Jul 31, 2017

Should use thrown(InvalidUserDataException).

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactoryTest.groovy

+                      when:
+                          factory.createSignatory(project, true)
+                      then:
+                          //notThrown NullPointerException // does not allow the real exception through

Contributor

bmuschko Jul 31, 2017

I think we should check the actual exception type here. In fact I think we can delete this test case as it is covered by null properties throw a descriptive exception. Please also see my comment below on asserting the correct exception.

...ning/src/test/groovy/org/gradle/plugins/signing/signatory/pgp/PgpSignatoryFactoryTest.groovy

+              import org.gradle.api.InvalidUserDataException
+              import org.gradle.api.Project
+              import org.gradle.testfixtures.ProjectBuilder
+              import spock.lang.*

Contributor

bmuschko Jul 31, 2017

Can you run "optimize imports" from the IDE here?

Contributor

bmuschko commented Aug 1, 2017

@colindean Thanks again for your fixes. I am going to finish off the PR. No more need for any changes from your end.

bmuschko merged commit 5eb7335 into gradle:master

Contributor Author

colindean commented Aug 1, 2017

Thank you! I would have liked to do more but I'm a little time limited. I was working on this during some funemployment!

colindean deleted the colindean/check-null-signing-properties-gh2267 branch

August 1, 2017 18:48

bmuschko changed the title ~~Adds a check for null values in signing properties~~ Prevent NullPointerException if any of the signing properties is null but signing isn't required

bmuschko added a commit that referenced this pull request


          Polish changes from pull request #2268

bmuschko added this to the 4.2 RC1 milestone

bmuschko removed the waiting-for:contributor label

eriwen added the was:delivered label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment