[
https://issues.apache.org/jira/browse/BROOKLYN-129?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14293494#comment-14293494
]
ASF GitHub Bot commented on BROOKLYN-129:
-----------------------------------------
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71647237
@michaeldye @sjcorbett The current behaviour (before this PR) is definitely
wrong - of appending `null` to the URL.
Whether or not it should filter nulls automatically is a stylistic
discussion for Brooklyn APIs. Unfortunately we have a bit of a mix - e.g.
`brooklyn.util.collections.MutableList` creation methods take `null` to mean
creating an empty list; other places in our code follows the guava
best-practice of treating `null` as a programming error (failing fast, to catch
these more easily).
In this situation, I lean towards @sjcorbett 's suggestion - a null in the
middle of an array of strings smacks of a programming error, rather than a
short-hand convenience for saying "create an empty thing". The caller can
always filter out the nulls if it truly is valid to have nulls in that context.
Note that passing in an empty string is valid - that will effectively be
ignored.
@michaeldye does that make sense for your use-case, if this were to throw a
`NullPointerException` when the vararg array of `items` contains a `null`?
p.s. in case anyone is interested in the history, one reason we accept
nulls in things like `MutableMap` etc is because of the handling of config and
attributes. The values supplied by the user can include explicit nulls, which
leads to NPEs if you try to just use `ImmutableMap.copyOf(...)`.
> brooklyn.util.net.Urls.mergePaths(String... items) doesn't filter null values
> -----------------------------------------------------------------------------
>
> Key: BROOKLYN-129
> URL: https://issues.apache.org/jira/browse/BROOKLYN-129
> Project: Brooklyn
> Issue Type: Bug
> Affects Versions: 0.7.0-M2
> Reporter: michael dye
> Fix For: 0.7.0-M2
>
> Original Estimate: 0.5h
> Remaining Estimate: 0.5h
>
> brooklyn.util.net.Urls.mergePaths(String... items) iterates over given array
> of paths and merges them, including null values. In some cases, this can lead
> to later evaluation of paths like "null/foo.tar.gz". Logging from error
> encountered while deploying from a Chef recipe:
> {noformat}
> 2015-01-20 20:55:31,652 WARN Error invoking start at
> BasicApplicationImpl{id=wjm4Zaws}: Error invoking start at
> BasicApplicationImpl{id=wjm4Zaws}: Error invoking start at
> ChefEntityImpl{id=AfLQVhJO}: SSH task ended with exit code 2 when 0 was
> required, in Task[ssh: extracting archive:hEupCHeQ]: extracting archive
> ...
> cd /root/brooklyn-managed-processes/installs/chef/tmp-brooklyn_doc_gen-ZWet
> tar xvfz null/brooklyn_doc_gen.tar.gz
> rm null/brooklyn_doc_gen.tar.gz
> {noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
