New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CsvSeq unmarshaller to include trailing empty string values in the result Seq[T] #2249

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
4 participants
@maximskripnik
Copy link
Contributor

maximskripnik commented Oct 10, 2018

Hello,

This PR solves the issue when CsvSeq[T] unmarshaller would ignore last empty string value (the one after trailing comma), i.e. would not pass it to underlying unmarshaller. This is agains csv semantic, so I think it is fair to assume that this is bug.

val values = Await.result(CsvSeq[String].apply(",value1,value2,"), Duration.Inf)
 // Current behaviour
assert(values == Seq("", "value1", "value2"))
// Expected behaviour
// assert(values == Seq("", "value1", "value2", ""))

This is my very first PR, so I am scared and sorry if I did something wrong, but the fix seems straightforward enough to try submitting it

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 10, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@johanandren
Copy link
Member

johanandren left a comment

Good catch. LGTM

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Oct 10, 2018

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels Oct 10, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 10, 2018

Test PASSed.

@raboof

raboof approved these changes Oct 11, 2018

Copy link
Member

raboof left a comment

Good contribution, thanks!

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Oct 11, 2018

We don't seem to have this feature documented, do we? Should we?

@maximskripnik

This comment has been minimized.

Copy link
Contributor

maximskripnik commented Oct 12, 2018

@raboof If you mean documentation for CsvSeq in general, there is: https://doc.akka.io/docs/akka-http/current/routing-dsl/directives/parameter-directives/parameters.html#csv-parameter
But it covers only String params, not any T (as long as this T is unmarshallable from String).
This feature was added by this commit, I believe, but it does not contain any updates in documentation.

We could add some samples (and tests) for custom types to be used in CsvSeq in addition to String, if that is what you're suggesting, I think it would be nice to have

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Oct 12, 2018

Ah, missed that. I agree showing that it works for T would be nice but that's something for another PR.

Thanks for this one!

@raboof raboof merged commit dd92133 into akka:master Oct 12, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@maximskripnik maximskripnik deleted the maximskripnik:fix-CsvSeq-unmarshaller branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment