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.

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

bindnode: fix for stringjoin struct emission when first field is the empty string #239

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

warpfork
Copy link
Collaborator

@warpfork warpfork commented Sep 2, 2021

In the case of an empty string as the first field, the buffer length
is not a valid proxy for whether we're on the first field or not.

This means if we have some type like:
type Foo struct {a String; b String} representation stringjoin(":"),
and the value of it is {"", "b"}, then the string of that
should still be ":b".

Before this fix, it would incorrectly be emitted as "b" (no joiner),
which would not round-trip.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

you know you want to add a test for this

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Nice catch, oops.

FYI Rod, with Eric we've been discussing testing. One half of it will be extending the schema test suite, and another half will be adding table-driven bindnode tests for anything that's specific to how it handles Go types.

In this particular case, I agree with Rod: this could just be an added test in the schema test suite, so there's no reason to delay it.

…empty string.

In the case of an empty string as the first field, the buffer length
is not a valid proxy for whether we're on the first field or not.

This means if we have some type like:
`type Foo struct {a String; b String} representation stringjoin(":")`,
and the value of it is `{"", "b"}`, then the string of that
should still be ":b".
Before this fix, it would incorrectly be emitted as "b" (no joiner),
which would not round-trip.

Includes regression test.
@warpfork warpfork force-pushed the bindnode-stringjoin-fix-for-first-field-empty branch from c6a744d to dbe1bef Compare September 3, 2021 10:56
@warpfork
Copy link
Collaborator Author

warpfork commented Sep 3, 2021

You're both right, this was easily added to the existing schema test suite, so I was indefensibly lazy not to add a regression test. Fixed.

(I yearn to get to data-driven tests that we can put in the ipld meta repo, but that's no excuse for not doing the best we can along the way until we get there.)

@warpfork warpfork merged commit c32e220 into master Sep 3, 2021
@warpfork warpfork deleted the bindnode-stringjoin-fix-for-first-field-empty branch September 3, 2021 10:59
@aschmahmann aschmahmann mentioned this pull request Sep 30, 2021
62 tasks
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.

3 participants