[net] internal/httpsfv: add parsing functionality for types defined in RFC 8941
Nicholas Husin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 3 comments![]( "Open in Gerrit")
Votes added by Damien Neil
| Code-Review | +2 |
3 comments
// Structured Field Values.Simpler:
ParseInteger parses an Integer value.
func ParseInteger(s string) (parsed int64, ok bool) {This is fine for the moment, but I think we're going to want to change this a bit if we externalize this package. (And I realize that the current function signature was my suggestion in the first place, so apologies.)
ParseInteger parses an Integer value. If a user is trying to parse a SFV header containing an Integer, it will be tempting to write something like:
```
v, ok := httpsfv.ParseInteger(header.Get("Some-Integer-Field"))
```
However, this will be wrong: It won't handle leading and trailing whitespace, and it won't handle attributes. We'll either need a `ParseIntegerField` function, or a `ParseItem` which returns a value which should subsequently be passed to `ParseInteger`.
I'm not sure what the right API is yet (and what we have here is fine for our immediate internal needs) but if we externalize this, we'll want to make it hard to accidentally do the wrong thing.
// ParseDecimal used to parse a string that represents a decimal in an HTTPMissing "is", but "ParseDecimal parses a Decimal value" is simpler.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 3 comments![]( "Open in Gerrit")
Simpler:
ParseInteger parses an Integer value.
Thanks! Updated it along with all other comments.
func ParseInteger(s string) (parsed int64, ok bool) {This is fine for the moment, but I think we're going to want to change this a bit if we externalize this package. (And I realize that the current function signature was my suggestion in the first place, so apologies.)
ParseInteger parses an Integer value. If a user is trying to parse a SFV header containing an Integer, it will be tempting to write something like:
```
v, ok := httpsfv.ParseInteger(header.Get("Some-Integer-Field"))
```However, this will be wrong: It won't handle leading and trailing whitespace, and it won't handle attributes. We'll either need a `ParseIntegerField` function, or a `ParseItem` which returns a value which should subsequently be passed to `ParseInteger`.
I'm not sure what the right API is yet (and what we have here is fine for our immediate internal needs) but if we externalize this, we'll want to make it hard to accidentally do the wrong thing.
Hmm, that makes sense...
We'll either need a ParseIntegerField function, or a ParseItem which returns a value which should subsequently be passed to ParseInteger.
We do have a `ParseItem` already. Although, I'm not sure it'll help much with the temptation you outlined (if I imagine myself as a user wanting to get an integer, I'd probably look for `ParseInteger` first and roll with that before I found `ParseItem`). Item parsing also do not allow leading whitespace, following the spec.
Some initial ideas here:
- We don't expose individual `ParseXxx` functions, but instead just give `ParseBareItem(s string) (any, ParsedType)`, where `ParsedType` indicates which type we detected (`Integer`, `Decimal`, `Unknown` for error). So, the expectation is clear that we only expect bare items.
- We change the signatures to `ParseInteger(s string, mustParseAll bool) (parsed int64, ok bool)`. if `mustParseAll` is `false`, we don't return `ok = false` when we cannot consume the entire string. That way, the expectation is clearer to the caller, and they can choose to ignore attached parameters if they so desire.
- Maybe we can just rename these to `ParseBareXxx`? Or, expose different variants (at the risk of a large API surface, so leaning no here personally) such as `MustParseXxx` (errors out if it cannot consume all the string) and `ParseXxx`.
// ParseDecimal used to parse a string that represents a decimal in an HTTPMissing "is", but "ParseDecimal parses a Decimal value" is simpler.
Fixed, thanks!
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin voted Code-Review+1![]( "Open in Gerrit")
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Damien Neil
| Code-Review | +2 |
1 comment
func ParseInteger(s string) (parsed int64, ok bool) {Nicholas HusinThis is fine for the moment, but I think we're going to want to change this a bit if we externalize this package. (And I realize that the current function signature was my suggestion in the first place, so apologies.)
ParseInteger parses an Integer value. If a user is trying to parse a SFV header containing an Integer, it will be tempting to write something like:
```
v, ok := httpsfv.ParseInteger(header.Get("Some-Integer-Field"))
```However, this will be wrong: It won't handle leading and trailing whitespace, and it won't handle attributes. We'll either need a `ParseIntegerField` function, or a `ParseItem` which returns a value which should subsequently be passed to `ParseInteger`.
I'm not sure what the right API is yet (and what we have here is fine for our immediate internal needs) but if we externalize this, we'll want to make it hard to accidentally do the wrong thing.
Hmm, that makes sense...
We'll either need a ParseIntegerField function, or a ParseItem which returns a value which should subsequently be passed to ParseInteger.
We do have a `ParseItem` already. Although, I'm not sure it'll help much with the temptation you outlined (if I imagine myself as a user wanting to get an integer, I'd probably look for `ParseInteger` first and roll with that before I found `ParseItem`). Item parsing also do not allow leading whitespace, following the spec.
Some initial ideas here:
- We don't expose individual `ParseXxx` functions, but instead just give `ParseBareItem(s string) (any, ParsedType)`, where `ParsedType` indicates which type we detected (`Integer`, `Decimal`, `Unknown` for error). So, the expectation is clear that we only expect bare items.
- We change the signatures to `ParseInteger(s string, mustParseAll bool) (parsed int64, ok bool)`. if `mustParseAll` is `false`, we don't return `ok = false` when we cannot consume the entire string. That way, the expectation is clearer to the caller, and they can choose to ignore attached parameters if they so desire.
- Maybe we can just rename these to `ParseBareXxx`? Or, expose different variants (at the risk of a large API surface, so leaning no here personally) such as `MustParseXxx` (errors out if it cannot consume all the string) and `ParseXxx`.
`ParseBareInteger` etc. seems like good naming to make the behavior clear. We may as well do that.
For a final API (sorry, this is a lousy place to do design but I'm already typing), one possibility is something like:
```
// item and attrs are strings.
// ParseItem discards leading and trailing whitespace and splits off the attribute.
item, attrs, err := httpsfv.ParseItem(value)
// try to parse item
s, err := httpsfv.ParseBareString(item)
```
Another is to introduce an item type:
```
// item is an httpsfv.Item
item, err := httpsfv.ParseItem(value)
// iterate over the item's attributes
item.Attributes(func(key string, value Item) {
})
// try to interpret item as a String
s, err := item.String()
```
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
internal/httpsfv: add parsing functionality for types defined in RFC 8941
This change introduces parsing functions for all item types defined in
RFC 8941, namely: integers, decimals, strings, tokens, byte sequences,
and booleans.
At this point, internal/httpsfv should be usable for parsing any RFC
8941-compliant HTTP Structured Field Values.
In a future CL, we will add support for parsing display strings and
dates, so that this package fully supports RFC 9651.
For golang/go#75500
- M internal/httpsfv/httpsfv.go
- M internal/httpsfv/httpsfv_test.go
Code-Review: +2 by Damien Neil, +1 by Nicholas Husin
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |