You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by michaeldye <gi...@git.apache.org> on 2015/01/26 17:42:23 UTC
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
GitHub user michaeldye opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/471
fixed BROOKLYN-129: brooklyn.util.net.Urls.mergePaths(String... items) doesn't filter null values
Fix for [BROOKLYN-129](https://issues.apache.org/jira/browse/BROOKLYN-129): filter nulls before iterating through path parts.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaeldye/incubator-brooklyn bug/BROOKLYN-129
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/471.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #471
----
commit 0175b95b0748ece05da3471559dac11e534d422b
Author: michaeldye <m-...@divisive.info>
Date: 2015-01-26T16:35:08Z
fixed BROOKLYN-129: brooklyn.util.net.Urls.mergePaths(String... items) doesn't filter null values
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
Posted by michaeldye <gi...@git.apache.org>.
Github user michaeldye commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71680008
@sjcorbett @aledsage Thank you for the discussion. I agree that it really should be up to the caller to provide acceptable arguments to ```Urls.mergePaths(String... items)``` and that this method should merely enforce that contract by throwing an exception given a null arg. Since the null values in this case aren't dereferenced, but rather passed to ```StringBuilder.append()```, does an NPE make the most sense? How about an ```IllegalArgumentException``` and a message including the rest of the path to help someone debug it later?
If you agree, I'll update this PR with the behavior desired for mergePaths and create another PR with updates to the calling code. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71718907
@michaeldye I agree that: if any of the Strings in `items` is null, it should throw an exception that includes the rest of the path in its message.
I'd prefer a `NullPointerException` rather than an `IllegalArgumentException`. To quote Joshua Bloch's Effective Java item 60: "If a caller passes null in some parameter for which null values are prohibited, convention dictates that NullPointerException be thrown rather than IllegalArgumentException."
You can call the NPE constructor that takes a String, so can still pass in the message.
If you can update the PR accordingly (and ideally add a test case to `brooklyn.util.net.UrlsTest`), then that would be fantastic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71644193
_Should_ it filter nulls? It strikes me as an error of the caller and something that would be quite surprising in practice. What is the context of the original error in BROOKLYN-129?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.util.net.Urls.mergePaths...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-72004063
Looks good; merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.util.net.Urls.mergePaths...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/471
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
Posted by aledsage <gi...@git.apache.org>.
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(...)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fixed BROOKLYN-129: brooklyn.util...
Posted by michaeldye <gi...@git.apache.org>.
Github user michaeldye commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71870374
Sounds good, and thanks for the note on the NPE convention. I've updated the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---