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

fix(test): rootless fixture should have no roots, not null roots #293

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 3, 2022

I was trying to use this as a fixture and it's causing a bit of a problem. What we want this fixture to test is that the version or 42, so the error is invalid car version: 42. But, because it's got a Null roots it violates the schema in a novel way. The schemas for CARv1 headers dictate that the roots field is strictly necessary, and not nullable, but for the CARv2 pragma, we remove it entirely. So this effectively makes the initial bytes schema something like:

type CarHeader struct {
  version Int
  roots optional [&Any]
}

So it serves both the CARv2 pragma, and the CARv1 header proper.

But this fixture would make it:

type CarHeader struct {
  version Int
  roots nullable optional [&Any]
}

Which introduces additional awkwardness.

So I'd like to hand-wave this problem away entirely.

I'll also note here that it's a bit too easy at the moment to write a CARv1 with a Null in this position, WriteCar() and friends, and ReplaceRootsInFile() in CARv2 just pass the roots value on to cbor.DumpObject() which doesn't care about these nuances, it'll write a nil as a Null. I'm thinking that maybe we should switch from go-ipld-cbor to bindnode and assert a strict schema for the header to make this go away. The alternative is to add a nullable to the CARv1 spec (I started a PR to do this but then thought that maybe it'd be better to sweep this fixture under the carpet and defer this problem till later).

Copy link
Member

@masih masih left a comment

Choose a reason for hiding this comment

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

Thank you sir 🍻

@mvdan mvdan merged commit d2cce98 into master Mar 3, 2022
@mvdan mvdan deleted the rvagg/fix-rootless-fixture branch March 3, 2022 10:45
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