(RESTCONF+YANG) PUT Request with invalid JSON modifies state

[sibuk-harabudjasim]

When making CRUD request (PUT) to RESTCONF with invalid JSON (missing bracket for ex), request is rejected with proper error message but state is also changed. I found core of a problem, but don't have proper solution.
IMO problem lays between these 3 functions:

yang/nodeutil/json_rdr.go

Line 21 in fe20e07

func ReadJSONIO(rdr io.Reader) node.Node {

func ReadJSONIO(rdr io.Reader) node.Node {
	jrdr := &JSONRdr{In: rdr}
	return jrdr.Node()
}

Here function returns node.Node (which can be node.ErrorNode in case of read errors) without separate error value.
This cause code in restconf:
https://github.com/freeconf/restconf/blob/72cc856c177bf129d9c55bb2978367379209b4c6/browser_handler.go#L179-L184

		case "PUT":
			// CRUD - Remove and replace
			var input node.Node
			input, err = requestNode(r)
			if err != nil {
				handleErr(compliance, err, r, w)
				return
			}
			err = target.ReplaceFrom(input)

to not to check error and pass erroneous node.Noderight to yanglibrary.

Here, ReplaceFrom first deletes previous version of element before trying to apply new version which is an just error.

yang/node/selection.go

Line 470 in fe20e07

func (sel *Selection) ReplaceFrom(fromNode Node) error {

func (sel *Selection) ReplaceFrom(fromNode Node) error {
	parent := sel.parent
	if err := sel.Delete(); err != nil {
		return err
	}
	return parent.InsertFrom(fromNode)
}

The error is returned but it's too late, data is already gone.

Like I said, I've done investigation :) but I don't know how to properly fix this issue to keep code consistency.
Main question is: what is better, to return error from ReadJSONIO() or to check for node.ErrorNode in browser_handler.go?

[sibuk-harabudjasim]

Main root of a problem is solved in #101 PR, but IMO, ReplaceFrom function should also be refactored because data still can be lost if fromNode will be correct JSON but with different structure than expected.

[sibuk-harabudjasim]

Sorry, I have to reopen this issue.
I'd like to fix the problem I mentioned in #100 (comment)

data still can be lost if fromNode will be correct JSON but with different structure than expected.

I have time to do this but I need an advice how.

Core of a problem is in this function:

yang/node/selection.go

Line 470 in fe20e07

func (sel *Selection) ReplaceFrom(fromNode Node) error {

It modifies tree in 2 steps: sel.Delete() and parent.InsertFrom(), each is a separate action wrapped in BeginEdit-EndEdit, so it is even harder to detect issue with payload from application code.
I thought that we can copy selection before delete operation so we can restore previous state from it in case of insert errors, but such approach can be expensive due to copying. If you have any proposals about how to refactor this function, please, share them and I can try to prepare PR.

Many thanks.

[dhubler]

As it exists today FreeCONF's operations happen on the live objects. I think to achieve what you are looking for freeconf should support "data stores" that act as sandbox to validate requests and only after no errors are found, then reapply the request to the live objects.
But yeah, this validation can be expensive depending on how big the request scope is but in your case, you delete everything first, so not really a "copy" of the original but rather a start w/empty and apply input twice, once to sandbox once to live objects.