You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Roman Coedo <no...@github.com> on 2014/06/12 13:15:36 UTC
[jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
...now let the server return the error.
This PR addressed the issues discussed on https://github.com/jclouds/jclouds-labs-aws/pull/6
You can merge this Pull Request by running:
git pull https://github.com/rcoedo/jclouds-labs-aws upstream
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-aws/pull/7
-- Commit Summary --
* Instead of validating the vault names and the pagination parameters, we now let the server return the error.
-- File Changes --
M glacier/src/main/java/org/jclouds/glacier/GlacierAsyncClient.java (6)
M glacier/src/main/java/org/jclouds/glacier/options/PaginationOptions.java (6)
D glacier/src/main/java/org/jclouds/glacier/predicates/validators/VaultNameValidator.java (59)
D glacier/src/test/java/org/jclouds/glacier/predicates/validators/VaultNameValidatorTest.java (55)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-aws/pull/7.patch
https://github.com/jclouds/jclouds-labs-aws/pull/7.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Phillips <no...@github.com>.
@andrewgaul W.r.t. the `testIllegalCharacters` errors in your Gist, do you think the server _would_ return the correct error if we were to provide the "correct" signature?
It seems to me our signature-generator is perhaps not handling the unexpected name properly, and that's masking the "correct" server error that would perhaps otherwise be returned?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45971555
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Phillips <no...@github.com>.
> but I created equivalent live tests and do not believe the Glacier error messages are useful enough
> to remove this functionality.
That Gist is interesting, indeed! The one (name too long) error message seems to contain all the information we'd need:
```
java.lang.IllegalArgumentException: Invalid vault name: INVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVA
LIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAMEINVALIDNAME. Valid vault names may consist only of a-z, A-Z, 0-9 ".", "-", and "_". and have length limit of 255
```
Strange to see that Glacier doesn't return that message for illegal characters!
May I suggest a compromise solution: we remove the max-length check from our validator and use the same error message as the server probably _should_ be returning (`Valid vault names may consist only of a-z, A-Z, 0-9 ".", "-", and "_".`) for the other cases?
I'm happy to open and take care of that PR if we feel that's a good idea.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45971459
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #17](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/17/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45879886
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Gaul <no...@github.com>.
Wire logs show:
```
2014-06-13 15:42:52,220 DEBUG [jclouds.headers] (TestNG) >> PUT https://glacier.us-east-1.amazonaws.com/-/vaults/INVALID%2CNAME HTTP/1.1
```
but Glacier complains with:
```
The Canonical String for this request should have been
'PUT
/-/vaults/INVALID%252CNAME
```
This looks like Glacier doubled percent-encoded its input, from , to %2C to %252C)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-46068543
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Roman Coedo <no...@github.com>.
Sure, no problem.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45956506
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Gaul <no...@github.com>.
@rcoedo Sorry to reverse my opinion on this, but I created equivalent live tests and do not believe the Glacier error messages are useful enough to remove this functionality. The too long vault name delivers something readable but the empty and invalid characters do not as shown by this gist:
https://gist.github.com/andrewgaul/ba37dca754f2c1e58e57
Can we keep the existing functionality and close this pull request?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45955566
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Phillips <no...@github.com>.
> For the empty vault name, Glacier returns GlacierError code=BadRequest
Yes, for this one it definitely looks like some client-side validation will help.
> This looks like Glacier doubled percent-encoded its input, from , to %2C to %252C
Wait...am I understanding this correctly? Glacier is complaining that it would have expected an encoded form of something that _is already encoded_? Are we encoding something on the client side that Glacier expects **not** to be encoded?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-46137045
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Roman Coedo <no...@github.com>.
Closed #7.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#event-130951791
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-labs-aws #940](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/940/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-45879894
Re: [jclouds-labs-aws] Instead of validating the vault names and the
pagination parameters, we ... (#7)
Posted by Andrew Gaul <no...@github.com>.
For the empty vault name, Glacier returns GlacierError code=BadRequest instead of InvalidParameterValueException.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/7#issuecomment-46068895