(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: https://github.com/freeconf/yang/blob/fe20e073cbc56867505f15dce50379d88296b8f3/nodeutil/json_rdr.go#L21

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. https://github.com/freeconf/yang/blob/fe20e073cbc56867505f15dce50379d88296b8f3/node/selection.go#L470

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 https://github.com/freeconf/yang/issues/100#issuecomment-1900640027

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: https://github.com/freeconf/yang/blob/fe20e073cbc56867505f15dce50379d88296b8f3/node/selection.go#L470

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.