Skip to content

Conversation

akonradi-signal
Copy link
Contributor

This fixes a parsing bug that resulted in commas within quoted blocks being treated as delimiters for directives.

The test case added in the first commit demonstrates the issue, and is fixed by the second commit.

Cargo.toml Outdated
@@ -23,6 +23,7 @@ bytes = "1"
mime = "0.3.14"
sha1 = "0.10"
httpdate = "1"
either = "1.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of pulling in another dependency, I'd prefer if we used a custom enum, or something else local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I went with at first locally but it seemed like more trouble than it was worth. either is a relatively small crate and doesn't have any dependencies of its own so it seemed light-weight enough to include. If you're unconvinced I'm happy to write a custom enum.

Copy link
Member

Choose a reason for hiding this comment

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

Well, before I suggest that, I want to better understand. It seems like the functions/types in util::csv should work, it has tests for quoted strings... If not, should this behavior be limited to only cache-control?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. It looks like util::csv and util::flat_csv behave differently. I suspect csv should make use of the same quoted handling as flat_csv...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy enough, added quoted block handling to csv parsing.

@seanmonstar seanmonstar merged commit 0a87dfc into hyperium:master Aug 18, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants