[BEAM-14405] Fix NPE when ProjectID is not specified in a template execution

[nielm]

PR #16547 added handling for null ProjectIDs on command line execution,
but did not handle the possibility of null ProjectIDs in template
execution. PR #17094 added nullness checks in MonitoringInfoMetricNames,
which then triggered NPEs if the ProjectID was not specified during a
template execution.

This corrects this by checking for a null ValueProvider, a null value in the ValueProvider, and an empty String in the ValueProvider.

---

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[asf-ci]

Can one of the admins verify this patch?

[asf-ci]

Can one of the admins verify this patch?

[nielm]

R: @lukecwik

[nielm]

Run Java PreCommit

[chamikaramj]

Thanks. LGTM.

[nielm]

One minor update - added an .isEmpty() check for the projectID value (as well as nullness).

[nielm]

Run Java PreCommit

[kennknowles]

Would be great to follow up by removing the suppressions that made NPE possible in the first place, or we should presume there are others lurking.

[nielm]

I am not sure what you mean by 'supressions' here?
(This issue is unrelated to the suppression of runtime exceptions in Spanner, GCS, DataStore connectors)

The NPE was introduced in 2.35 (along with the other issue) by the implementor of the MontoringInfoMetrics, who assumed that the projectId could never be null.

I fixed this in 2.37, but assumed naively/incorrectly that the projectID valueProvider object would always be null when a value was not present - which is normally true for command line executions but not for template executions (nor when a user explicitly passes a null value).

This was not noticed until an unrelated change used an ImmutableMap for MontoringInfoMetricName keys, which enforced non-null values (in 2.38).

So yes it is still possible that there are various NPEs waiting to be found in other IOs where MonitoringInfoMetrics were added.

[kennknowles]

@SuppressWarnings("nullness") is what I mean by suppressions.

[kennknowles]

We use checkerframework, which is a full type system (not heuristic) that prevents NPEs wherever it is not disabled.

[aaltay]

Should this be mentioned in the release notes for 2.38.0 as a known issue?

[chamikaramj]

Currently Beam release notes seems to be a list of auto generated JIRAs created at release time. For example, https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12319527&version=12351169

Is there a better place to record known issues found after that fact ?

[kennknowles]

There's CHANGES.md and the blog post and the affected versions field in the Jira. I don't know if we point users to these very much.

[chamikaramj]

CHANGES.md seems to be the best candidate. Sent #17631 to update it on this and other recent issues.