/fetch

New issue

Header value definition needs work #332

Open
jeenalee opened this Issue Jul 12, 2016 · 7 comments

Labels

None yet

Milestone

No milestone

Assignees

No one assigned

5 participants

@jeenalee @malisas @annevk @mnot @reschke
@jeenalee
jeenalee commented Jul 12, 2016

The spec defines header value as "a byte sequence that matches the field-content token production" [0, 1].

Field-content is defined as field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] [2]. This seems to say that field-content is at most a field-vchar followed by some whitespace and another field-vchar. It seems wrong that it would be at most 2 vchars.

Should the Fetch spec define header value as "a byte sequence that matches the field-value token production"? Field-value token production is defined as field-value = *( field-content / obs-fold ) [2].

Thank you.

[0] https://fetch.spec.whatwg.org/#concept-header-name
[1] https://github.com/whatwg/fetch/blob/master/Overview.html#L440
[2] https://tools.ietf.org/html/rfc7230#section-3.2

@jeenalee
jeenalee commented Jul 13, 2016

We found an errata today, which defines field-content = field-vchar [ 1*( SP / HTAB / field-vchar ) field-vchar ] [0].

Did you mean that the header value should match the field-content production in the errata?

[0] https://www.rfc-editor.org/errata_search.php?rfc=7230 (Errata ID: 4189)

@malisas
malisas commented Jul 15, 2016

How is this for field-content?

field-content = [field-vchar [*(field-vchar/SP/HTAB/obs-fold) field-vchar]]

The notes from the errata linked above says:

-what the authors propably wanted to say:
a string of octets is a field-value if, and only if:
-it is *( field-vchar / SP / HTAB / obs-fold )
-if it is not empty, it starts and ends with field-vchar

With this re-write, you can:
1) have an empty field-content string, or
2) have a single field-vchar, or
3) have two field-vchars sandwiching some optional content

I guess we then get rid of field-value altogether.

@jeenalee jeenalee referenced this issue in servo/servo Jul 15, 2016
Merged

Add the append method for the Headers API #12467

3 of 5 tasks complete
@annevk
WHATWG member
annevk commented Jul 19, 2016

Thank you for reporting this. I suspect it's a duplicate of either #213 or #115. Clearly something is wrong here. I also didn't know errata was filed for this particular production so thanks for pointing that out too.

@hiroshige-g have you looked into this since February? Should we just go back to what we had before HTTP was revised? Or at least something that means the same. This new production has just led to flurry of issues.

@bors-servo bors-servo added a commit to servo/servo that referenced this issue Jul 20, 2016
@bors-servo bors-servo Auto merge of #12467 - jeenalee:jeena-headersAPI, r=jdm 
Add the append method for the Headers API

<!-- Please describe your changes on the following line: -->
This commit adds the append method for the Headers API. @malisas and I are both contributors.

There are a few TODOs related:
- The script needs to parse the header value for certain header names to decide the header group it belongs
- There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332))

There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12467)
<!-- Reviewable:end -->
 0bcf291
@bors-servo bors-servo added a commit to servo/servo that referenced this issue Jul 20, 2016
@bors-servo bors-servo Auto merge of #12467 - jeenalee:jeena-headersAPI, r=jdm 
Add the append method for the Headers API

<!-- Please describe your changes on the following line: -->
This commit adds the append method for the Headers API. @malisas and I are both contributors.

There are a few TODOs related:
- The script needs to parse the header value for certain header names to decide the header group it belongs
- There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332))

There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12467)
<!-- Reviewable:end -->
 03fa7f0
@mnot
mnot commented Jul 22, 2016

Note that the errata you mention is "Held for Document Update", and the notes say:

This has been edited from the original report after discussion, but even this is not right. There's more here than can be reasonably fixed in an errata report, and the proper fix needs to be done in a revision of the document -- hence, "Held for Document Update". Note that this is a valid report, and that a fix is needed. The one above is the best approach for now, and a better fix will be developed in 7230bis.

@reschke, any comment?

@reschke
reschke commented Jul 22, 2016

@mnot --no, we haven't figured out the best fix yet.

@annevk
WHATWG member
annevk commented Aug 10, 2016

@reschke is there an open issue I can track? This has been tripping up multiple implementers now, I feel like I should add some pointers in Fetch even if it's non-final.

@reschke
reschke commented Aug 10, 2016

I opened httpwg/http11bis#19

@annevk annevk changed the title from Possible typo in header value definition to Header value definition needs work Aug 12, 2016
This was referenced Aug 12, 2016
Closed
Closed
@triple-underscore triple-underscore added a commit to triple-underscore/triple-underscore.github.io that referenced this issue Aug 14, 2016
@triple-underscore triple-underscore [Fetch] Header value definition needs work 
whatwg/fetch#332
 391e6c8
This was referenced Sep 7, 2016
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment