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.

Sign up for GitHub

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

Jump to bottom

fix nil-reference issue in QuotedString module #202

Closed
wants to merge 1 commit into from
Closed

fix nil-reference issue in QuotedString module #202

wants to merge 1 commit into from

Conversation

jfredett
Copy link

fixes #201, #quote will no longer throw a nil-ref when passed a nil argument.

also adds specs for Webmachine::QuotedString#quote and #unquote. I didn't see any existing specs in the spec folder. There was one case where I encoded the current behavior, but think that maybe it should not behave this way:

context 'when the string is nil' do
  subject { unquote(nil) }

  it { is_expected.to eq(nil) }
end

IMO, either this should return "", or quote(nil) should return nil, rather than "". The former is perhaps more convenient, the latter is so that the invariants

quote(unquote(str)) == str

and

unquote(quote(str)) == str

hold. Right now, if str is nil, it would get 'promoted' to an empty string. Not sure what is preferable there.

@jfredett
fix nil-reference issue in QuotedString module
a3a8a1f 
quote(nil) now does not through an error
@ghost
Copy link

ghost commented Oct 30, 2014

Right now, if str is nil, it would get 'promoted' to an empty string.

On first thought, I'd prefer unquote(nil) == nil and quote(nil) == nil, not neccessarily for backward-compatibility, but rather for avoiding possible side-effects.

OTOH, an empty header should be considered not-set by Webmachine, anyway. Any other opinions?

@seancribbs
Copy link
Member

Strictly speaking, nil and an empty String are different things. nil should always mean the header was not present, whereas an empty string is simply an empty header. So I'd prefer @lgierth's way... quoting/unquoting nil should return nil.

The thing is, that code should only called from places where we know the argument to be a String. If it's not, the problem should be fixed in the caller.

@jfredett
Copy link
Author

@seancribbs While working on my 'real' issue -- I seem to be getting a 415 regardless of how I have this resource set up -- it turns out that this wasn't the root-cause of my issue (the code was being executed only due to my debugging code, not webmachine itself (I think)). So I'm not sure what the call-site was/is that caused this. Regardless of whether the actual code is/should be changed, though, I think the tests are valuable and am happy to update them to encode the correct behavior if you want to make a call on that. Otherwise I'm happy to close this as it's not going to actually fix my issue.

@jfredett jfredett closed this Mar 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Webmachine::QuotedString#quote fails when trying to quote request.if_match when it's nil
2 participants
@jfredett @seancribbs