Validation for tasks created in upgradeDependencyPlugin

[wwilk]

Problem

A few of the tasks created in UpgradeDependencyPlugin depend on "dependency" property that is passed as argument when running the task, eg:
performVersionUpgrade -Pdependency="org.shipkit:shipkit:1.2.3"
Unfortunately if you run one of them without this property there is no meaningful error message, it can fail on NPE or some other error during execution phase.

You can find the documentation for upgrade-dependency here.

Proposed solution

We should add a simple validation checking if the property is null or empty. We have to do this validation at the beginning of execution of task (maybe task.doFirst() would be the best place), we don't want to fail at configuration phase because task might not be actually used.

The message to the user should be meaningful, and actionable (telling the user that they should add the property to task execution).

The tasks affected by this problem are all that depend on values of following fields of UpgradeDependencyExtension class: newVersion, dependencyName, dependencyGroup. These fields are extracted from "dependency" property.

[wwilk]

@micd Want to work on this one?

[micd]

Yes. I will take a deeper look when I have time.

[micd]

Some validation is currently done in DependencyNewVersionParser#isValid, but there's a null check before method invocation.

I would propose to create Precondition<String> which will validate dependency property and place it in one of those places:

• UpgradeDependencyPlugin#apply just after extracting the property from project. Then after checking Precondition#isTrue we may throw a GradleException or do it even within Precondition#isTrue.
• DependencyNewVersionParser#fillVersionUpgradeExtension we can move whole validation to precondition and throw GradleException when precondition returns false.

Those are two "entry points" of dependency property and we can validate it at the very beginning.
I like second solution more.

What do you think @wwilk ?

[wwilk]

If you set it in UpgradeDependencyPlugin#apply validation will fail Gradle configuration phase even if task is never executed. You can try implementing that and then running ./gradlew tasks. It will fail because of lacking "dependency" property, although tasks task doesn't use it.

Validation in DependencyNewVersion can happen during the configuration phase because it basically checks if the format of "dependency" property is correct. If you use this property and it's incorrect then we should fail as soon as possible, to give the user actionable message what they should fix.

What we want is for the task to fail when it's executed without "dependency", so during the Gradle configuration phase. Here is more about Gradle build lifecycle. As I wrote in the description above it would probably be best to add validation to every affected task by task.doFirst(). See here for more information.

Let me know if you have any more questions.

[micd]

I was not familiar with Gradle phases. Thank you for explanation.

Taking this into consideration I would extend doFirst method in those tasks:

• ReplaceVersionTask
• CreatePullRequestTask

For this part we can cast Task returned by super.doFirst() to the current task retrieve UpgradeDependencyExtension and pass it to new precondition class UpgradeDependecyExists which will check if fields newVersion, dependencyName, dependencyGroup are null or empty. If yes throw GradleException if not return true in Precondition#isTrue

Additionally I would add validation to tasks definition in UpgradeDependencyPlugin:

• GitPushTask
• GitCheckOutTask
• FindOpenPullRequestTask
• ShipkitExecTask

For this part we can add null or empty check of passed parameters to UpgradeDependencyPlugin#getVersionBranchName. If one of them is null or empty we would need to throw the same GradleException similar to the one thrown in UpgradeDependecyExists.

For performVersionUpgrade definition inside UpgradeDependencyPlugin we should add validation using UpgradeDependecyExists precondition.

What do you think about this solution, @wwilk?

[wwilk]

Hey @micd
Sorry for the delay, busy time lately.

Thanks for the investigation, good points! After longer consideration I think the problem here is that different tasks use different properties of *dependency, but I think we don't need to distinguish that - the most important thing to achieve is to inform the user that the task failed because they didn't provide dependency.

The easiest way to validate if gradle task properties are not null is to use @input annotation. It will fail the task at the beginning of its execution, probably before doFirst methods, we would have to check that. The problem with this approach is that it doesn't tell us why this property is null. Eg user would know that ReplaceVersionTask failed because dependencyGroup was null, but wouldn't have a clue that it's due to missing dependency.

Validation in doFirst is fine, but the problem is that it probably fail faster if we use @input annotations, and we will use them eventually.

I think the best solution would be to use lazyConfiguration. See here.. There is a lot of exemplary usages in the code base.

So in short:

• for each task that depends (directly or indirectly) on dependency we add lazyConfiguration call
• inside lazyConfiguration callback we add validation that checks if dependency was set and fail if it wasn't
• inform the user that the dependency is missing.

[micd]

Hello @wwilk !
No problem.

Thanks for another possible solution!

I have just checked and LazyConfiguration will fail faster than @Input. I think this would be the "fail-fastest" solution here.

I would extend UpgradeDependencyPlugin with a new method which will add task to LazyConfiguration (and create a Runnable to do the validation). All tasks that depends on UpgradeDependencyExtension:

• GitPushTask,
• FindOpenPullRequestTask,
• ShipkitExecTask,
• ReplaceVersionTask,
• CreatePullRequestTask

should be passed to the new method.

In the new method dependency variable from UpgradeDependencyPlugin#apply should be checked. If it is null then throw GradleException.

[mockitoguy]

Hey guys! Can we close this ticket now? I see that the PR was merged.

[micd]

Hi @mockitoguy. I think that you can close it.