brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (BROOKLYN-129) brooklyn.util.net.Urls.mergePaths(String... items) doesn't filter null values
Date Tue, 27 Jan 2015 13:29:34 GMT

    [ 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)

Mime
View raw message