Skip to content
This repository was archived by the owner on Jul 31, 2025. It is now read-only.

Conversation

t-santoshsahu
Copy link
Collaborator

No description provided.

Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +127 to +129
func (d *Decoder) SetAllowMissingFields(v bool) {
d.allowMissingFields = v
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new public API function should have documentation explaining its purpose and behavior, similar to other public methods in this file.

Comment on lines +351 to +354
if d.allowMissingFields {
d.buf = buf
return d.callEmit(HeaderField{Name: fmt.Sprintf("missing-index-%d", idx), Value: "none"})
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an inconsistency in how missing fields are handled. In parseFieldIndexed(), you set the value to "none", but in parseFieldLiteral(), no value is set for missing fields. Consider using a consistent approach for both cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is if complete header filed is indexed. (key and value)
For parseFieldLiteral the value is not indexed, only header key

@tim-mwangi
Copy link

@t-santoshsahu are we actively pursuing this?

ihf, ok := d.at(nameIdx)
if !ok {
return DecodingError{InvalidIndexError(nameIdx)}
if !d.allowMissingFields {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have some unit tests to verify this logic.

@tim-mwangi
Copy link

We should probably have a readme on how to maintain it.

@t-santoshsahu
Copy link
Collaborator Author

@t-santoshsahu are we actively pursuing this?

Not actively pursuing it. I will get back to it. There seems to be an issue when there is packet loss, hpack indexing can cause mismatch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants