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

Patch spec #187

Merged
merged 8 commits into from Jun 6, 2022
Merged

Patch spec #187

merged 8 commits into from Jun 6, 2022

Conversation

warpfork
Copy link
Contributor

@warpfork warpfork commented Mar 6, 2022

Introducing a "patch" system, and defining it by fixtures!

This is roughly following the JSON Patch (RFC 6902) specification -- It's a very decent specification. (Most of the fixtures here also come from its examples appendix, sometimes with minor modifications.)

In brief highlights:

  • There are six operations -- add, replace, remove, copy, move, test.
    • (This is the same as RFC 6902.)
  • Operations are supplied in a list, and applied linearly. Any operation erroring results in the entire patch not being applied.
    • (This is the same as RFC 6902.)
  • Operations do not create parent paths automatically.
    • (This is the same as RFC 6902.)
    • (I think we may want to add ways to support this succinctly in the future!)
  • Note there is no "upsert" operation.
    • (This is the same as RFC 6902.)
    • (I think we may want to add ways to support this succinctly in the future!)
  • All of these fixtures are JSON -- but of course, because this is IPLD, both the subject data, as well as the patch instructions themselves, can be communicated in any codec (e.g. CBOR, or dag-pb (... for the subject data anyway), or etc).
  • Note that we do not follow RFC 6902's escaping rules (which appear to be from RFC 6901, although this is not explicitly stated). The opinion among us who did an early design review for this system was that the escaping mechanism shown there is very unusual, and would surprise most users, and thus we will not replicate it.
  • Note that a Patch system in IPLD is expected to maintain order whenever possible. Wherever this is ambiguous (e.g. operations adding data to a map under a key that didn't previously exist), operations are defined as appending to the end of the relevant node. This results in some of our fixtures being mildly divergent from RFC 6902 (which appears to treat map order as inconsequential). We consider this important to specify for determinism reasons.

There is a matching implementation, which exercises these fixtures, found in ipld/go-ipld-prime#350 .

@warpfork warpfork mentioned this pull request Mar 6, 2022
15 tasks
@warpfork
Copy link
Contributor Author

warpfork commented Mar 6, 2022

I think this is mergable. I'd like to add even more fixtures in the future (especially, some that examine how scenarios unfold when a patch fails to apply), and expand the system to include whole instruction message formats for applying patches to documents starting at a CID, and so forth -- but these fixtures here so far should all remain pretty incontrovertable even as we continue expanding the system, I think.

Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

This looks really handy!

I think it'd be nice if the examples contained one or two more complex structures like adding a Link to a UnixFS directory.

Also, do the operations work with other Kinds like Bytes? Can I delete a single byte from the middle?

Could be possible to track as TODOs for the future, too.

@BigLep BigLep requested a review from mvdan March 8, 2022 15:27
specs/patch/fixtures/fixtures-1.md Outdated Show resolved Hide resolved
@warpfork
Copy link
Contributor Author

I think it'd be nice if the examples contained one or two more complex structures like adding a Link to a UnixFS directory.

Yeah, I definitely want multi-block fixtures.

I dunno about UnixFS; it would be nice, but that hits a deeper dependency tree. (Some other PRs have done this recently, but it prompted a discussion already about practical maintainability, and I dunno if we have ideal answers yet.) We also don't necessarily want to suggest that that's a trivial thing that people should be doing bare; ideally that goes through an ADL code path so that sharding works transparently. (We've had bad experiences in the wild in the past with UnixFS directories being patched until they become too large to fit in the transfer size limit for a block; whoops.)

Also, do the operations work with other Kinds like Bytes? Can I delete a single byte from the middle?

Not yet. I think we'd have to introduce new operations for that.

The JSON Patch similarly doesn't specify such a feature for operating on strings.

We certainly could add another op code for that. But I probably won't try to heap that into this first pass.

@BigLep
Copy link
Contributor

BigLep commented Mar 23, 2022

This was fun to read. Thanks @warpfork . It looks like you got approval. Are we good to merge?

Also, for someone validating their implementation, do we need to provide more intricate structures (more nestings, maps inside lists, etc.)?

@BigLep BigLep assigned RangerMauve and unassigned warpfork May 3, 2022
@rvagg rvagg mentioned this pull request May 3, 2022
This patching system will be based heavily on JSON Patch (RFC 6902).
Many examples will come from that RFC.

Some divergences will also occur.  For example, these fixtures already
include some assertions that map order will be stable, which was not
maintained in RFC 6902.  It is also likely that we will *not* use the
path escaping system specified in RFC 6902 (and moreso in RFC 6901);
to use tildes for escaping seems *very* strange.

An implementation is developed in go-ipld-prime in parallel with
the growth of these fixtures.  Other implementations should be able
to follow suit in the future.
(The fixtures are still doing most of the real communication work here,
but *some* content is necessary here -- humans need an intro funnel...
and also, our tooling will yell if there are no pages here, heh:
the nav links get generated, but would be broken without content here.
(Nice, actually.))
@RangerMauve RangerMauve merged commit 9582ec2 into master Jun 6, 2022
@RangerMauve RangerMauve deleted the patch-spec branch June 6, 2022 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants