patch: first draft.

[warpfork]

(Work in progress.)

The JSON Patch specification is pretty decent; this roughly follows it.

Most of the heavy lifting is done by shelling out to the existing
traversal.FocusedTransform functions. We're just gluing that to a more
declarative API, which is more ready to be used via a serial API.

The performance of this system in the face of multiple patch operations
on the same target is probably abysmal. This implementation is an MVP.
It's likely that a smarter implementation could combine more than one
operation application onto the same data build process, and thus
induce a lot fewer copies. This also gets much trickier to implement
(maybe you need some patricia trees for ordering operations? or more
mutable nodes for the duration of the computation, then ideally a way
to freeze them again at the end so we don't break other consistency?),
so it's "future work". The goal of this changeset is probably going to
be limited in scope to getting the declarative API part hammered out.

As of this first commit, there's still several TODOs, regarding validation of
the patch instructions during their parse, generalizing the API to
support more codecs, and testing around upsets and moves and etc.

[warpfork]

This now appears to work with all six operations: add, replace, remove, copy, move, and test. Fixtures exist in the ipld/ipld repo for each.

[warpfork]

Probable future work:

• Even more fixtures! Especially those that probe error paths.
  • ... it might be nice to adopt the serum errors conventions here, and then as a bonus even get to use the go static analyzer for serum for the developer assistance and enforced high code quality.
• I'd like to standardize the error codes and how they're serialized.
  • ... it might be nice to do something about Improving clarity and error handling around absent values #360 first, because that's gnarly to try to paint over at a higher level.
• Tests still needed for patches that apply across links. (I'm fairly sure traversal.FocusedTransform, which I built this feature on, already supports this -- but it's not tested at this level.)
  • ... it might be nice to build some more helper features around go-testmark to help manage fixtures that are stored by content addressed hunk names (currently you can of course do this, but manually it would be a bit ishy to write such fixtures).
• There's still a variety of TODOs scattered in the implementation about better validation and error handling paths in general.
• The API for taking patch instruction messages (which have a CID to apply against, etc) and operating end-to-end returning a patch result message, etc -- those aren't included here yet. (There's some schema for them, but it's not all wired up.)

I'm not sure if these are worth doing immediately, or if this PR is worth landing as-is and iterating more in future PRs. (Leaning towards the latter, especially because having long-running branches in the ipld/ipld specs&meta repo becomes a bit frictional.)

[warpfork]

Status of this: I think I call this "alpha". The current behavior appears to be correct so far. The tests we have cover key features. I'd still consider it relatively experimental and the odds of bugs relatively high -- "use at your own risk". (I want many more test fixtures probing corner cases, in the long run, before promising complete stability.)

I think we may be able to merge this, with those caveats.

[BigLep]

What are the next steps here? Can this be merged after feedback is incorporated?

[RangerMauve]

TODO post merge: Update ipld.io/docs/intro/hello-world.md

[RangerMauve]

Two questions:

• I'm not seeing the errors for the operations when I try to run the tests locally on my machine. What can I do to debug this? Is it a golang version? I'm on go version go1.17.9 linux/amd64.

go test ./traversal/patch
ok  	github.com/ipld/go-ipld-prime/traversal/patch	(cached)

• What can I do about the following errors I'm getting when running go test ./...?

--- FAIL: TestRoundtripSchemaSchema (0.00s)
    roundtrip_test.go:23: 
        error:
          got non-nil error
        got:
          e"open ../../.ipld/specs/schemas/schema-schema.ipldsch.json: no such file or directory"
        stack:
          /home/mauve/programming/go-ipld-prime/schema/dmt/roundtrip_test.go:23
            qt.Assert(t, err, qt.IsNil)
        
FAIL
FAIL	github.com/ipld/go-ipld-prime/schema/dmt	0.002s
--- FAIL: TestParseSchemaSchema (0.00s)
    parse_test.go:30: 
        error:
          got non-nil error
        got:
          e"open ../../.ipld/specs/schemas/schema-schema.ipldsch: no such file or directory"
        stack:
          /home/mauve/programming/go-ipld-prime/schema/dsl/parse_test.go:30
            qt.Assert(t, err, qt.IsNil)
        
--- FAIL: TestParse (0.00s)
    parse_test.go:63: 
        error:
          unexpected success
        len(got):
          int(0)
        got:
          []string(nil)
        want length:
          <same as "len(got)">
        stack:
          /home/mauve/programming/go-ipld-prime/schema/dsl/parse_test.go:63
            qt.Assert(t, matches, qt.Not(qt.HasLen), 0)
        
FAIL
FAIL	github.com/ipld/go-ipld-prime/schema/dsl	0.003s

[rvagg]

@RangerMauve we have a git submodule sync problem here; both ipld/ipld and master have moved on from the last commit here.

If you run git submodule update --init --recursive you should reset back to the submodule commit this branch has and you should get the errors CI is showing here.

I think what you probably should do is rebase both ipld/ipld#187 and then here against current master versions and get them both in sync.

Then, if you replace your enum in the schema with:

		type Op enum {
			| add
			| remove
			| replace
			| move
			| copy
			| test
		}

it should work OK, bindnode is using the strings "Op_Add" etc. and they're just getting converted straight to Ops as Op("Op_Add") which is failing in the switch in eval.go.

Also as an aside, now I look at this conversion code in parse.go, I reckon I can make operationRaw go away with #414, since the main problem seems to be the conversion of string to datamodel.Path. So it's nice to see another use-case for doing that to avoid intermediate types.

[rvagg]

Also, the embed trick might be worthwhile here, see: https://github.com/ipfs/go-graphsync/tree/main/message/ipldbind

import _ "embed" then you can //go:embed schema.ipldsch a file into []byte so you can keep the schema file separate and it gets loaded automatically.

[rvagg]

I've cleaned this up a bit:

• rebased to master
• fixed the enum
• extracted the schema to a separate file
• cleaned up bindnode use a bit
• added an "EXPERIMENTAL" notice at the top of the package
• updated Patch spec ipld#187 (rebased to ipld/ipld master and updated submodule commit here); we have a green.

I reckon this might be mergeable along with ipld/ipld#187, what do you reckon @RangerMauve?

[RangerMauve]

Thanks @rvagg! I was away last week so I'm just getting into this now.

I was thinking some link traversal tests would be useful for patch before folks start building on it, but I guess that could be done in a separate PR so we can get this out the door. 🤔

[RangerMauve]

Also, I'm seeing a bunch of warnings saying that there's lines not covered by tests. Is that something we should address or is it expected?

[rvagg]

Also, I'm seeing a bunch of warnings saying that there's lines not covered by tests. Is that something we should address or is it expected?

The codecov is new, comes with the rebase to master where we now have the unified-ci workflow! So we get to feel that little bit more guilty about poor test coverage.

The uncovered bits are all for error cases, which are pretty annoying to cover in their entirety; and for a first pass at an experimental feature I don't think it's worth chasing those down here - at least that's not been the past practice in this repo where coverage hasn't been a super high priority. The one case where it's not for errors is the "-" path segment case where there's two paths where that would invoke if it were tested. It's in the comments:

There's also a special case for "-", which means "append to the end of the list".

But no tests for it. We maybe should add one for that since we have two codepaths that are relevant.

[RangerMauve]

Tests pass, merging.