Skip to content
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

Empty strings as valid values for URI parameters #72

Closed
wants to merge 3 commits into from

Conversation

honzajavorek
Copy link
Contributor

@honzajavorek honzajavorek commented May 18, 2017

⚠️ Work in progress. Do not merge.


This is my attempt to finish the work started in #71 by @motoyasu-saburi and described in apiaryio/dredd#677 and #70. It turns out this is much larger problem then anticipated. It is a breaking change in how Dredd works and it has many corner cases:

  • required/optional
  • default/example
  • null/undefined/empty string
  • query/path parameters

Regarding the last point, e.g. we do not want /sth/{param}/sth to work with empty strings, but we do want /sth{?param} to work with empty strings. I suggest we clearly specify all the corner cases (similar to #71 (review)) first, then write a lot of tests and then let's change the implementation and release the next major version of Dredd.


@motoyasu-saburi Thank you very much for bringing the whole thing up. You did awesome load of work. No matter any delays, your contribution in the issues and in the PR you started was a great help in understanding the whole problem and will be eternally carved in the history of Dredd development 🙇

motoyasu-saburi and others added 3 commits May 18, 2017 15:45
BREAKING CHANGE: Empty strings are now treated as valid values
for URI parameters.
@honzajavorek honzajavorek mentioned this pull request May 18, 2017
Closed
@motoyasu-saburi
Copy link

motoyasu-saburi commented May 18, 2017

I'm sorry.
I couldn't finish this,
because It's problem is large range of influence 😭

I had doubts when challenged fix this problem.

This condition false and true is String.
but I thought param.example and param.default setted to Boolean. 🤔

when 'boolean'
        if param.example isnt 'true' and param.example isnt 'false'

https://github.com/apiaryio/dredd-transactions/blob/master/src/validate-parameters.coffee#L16

P.S.
It was my first OSS activities, I am sorry to cause you inconvenience: bow:

I installing Dredd to the a company project now, and doing missionary of Dredd :) (https://www.slideshare.net/dcubeio/api-blueprint-75780293)
Thank you for develop good project.

Also, I try to challenge this bug.
It seems to easy.
(If I can not be done, I ask for help.)

apiaryio/dredd#709

@honzajavorek
Copy link
Contributor Author

@motoyasu-saburi No inconvenience at all! I'm very so happy you decided to contribute! It's actually a shame this is much larger problem than I expected. The presentation is awesome! You are doing a great work! 🙇

@honzajavorek
Copy link
Contributor Author

@motoyasu-saburi Would you mind sending me your e-mail (to [email protected])? I'd like to send you some t-shirts/stickers for all the amazing good you're doing for Dredd 🙂

@honzajavorek
Copy link
Contributor Author

We're decaffenaiting the codebase (#102, #126). Let's get back to this code as part of tackling the whole URI Parameters can of worms.

@honzajavorek honzajavorek deleted the honzajavorek/fix-empty-param-values branch November 21, 2017 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants