freeconf / yang

Standards-based management for Golang microservices
Apache License 2.0
38 stars 14 forks source link

Renaming of struct fields doesn't apply during replace operation? #67

Closed HRogge closed 1 year ago

HRogge commented 1 year ago

I noticed that a PUT (replace) operation doesn't run through the reflect.Child() code when looking for the relevant data for the JsonContainerReader.

The relevant code is json_rdr.go fqkGet() line 105/108, which does only test "name" and "mod.Ident():name"... Is there something that I am missing from the big picture as a reason why we don't end back in the reflect-code, where we call GetFieldName()?

HRogge commented 1 year ago

After stepping through a few more iterations of the problem I noticed that the browser_handler.go ServerHTTP() function (in its case "PUT") calls requestNode(),which in turn calls nodeutil.ReadJSONIO()... the result of this is a node.Node representation without the renaming GetFieldName() logic from reflect, which might lead the insert process mentioned above to miss the right struct fields?

dhubler commented 1 year ago

To help me understand better, would you mind rephrasing this as:

I sent {JSON here} my current config was {config here}. I expected {expected config here} but instead I got {actual config here}

BTW: PUT was actually acting like PATCH is supposed to up until a few months ago when I made it match RFC

On Wed, Jul 19, 2023 at 3:46 AM Henning Rogge @.***> wrote:

After stepping through a few more iterations of the problem I noticed that the browser_handler.go ServerHTTP() function (in its case "PUT") calls requestNode(),which in turn calls nodeutil.ReadJSONIO()... the result of this is a node.Node representation without the renaming GetFieldName() logic from reflect, which might lead the insert process mentioned above to miss the right struct fields?

— Reply to this email directly, view it on GitHub https://github.com/freeconf/yang/issues/67#issuecomment-1641587678, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7U6YRIJCVLPHAISTADXQ6GDJANCNFSM6AAAAAA2PPAKHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

HRogge commented 1 year ago

I am trying to overwrite my whole datamodel... and I am using the 'yang:"bla"' tags to define which YANG-fields map on which struct field.

But it seems when sending the PUT request to the restconf server it cannot find the correct struct fields, so it ends up ignoring the whole PUT request.

dhubler commented 1 year ago

I'm still having trouble. What is?

'yang:"bla"' tags

and what is

YANG-fields map on which struct field.

PUT ultimately calls

https://github.com/freeconf/yang/blob/master/node/selection.go#L510

Which does a delete, then an insert. I never tried this on the root level but I can see why that is useful and I can also see why the selection.go's implementation of deleting the root node is problematic.

On Wed, Jul 19, 2023 at 8:58 AM Henning Rogge @.***> wrote:

I am trying to overwrite my whole datamodel... and I am using the 'yang:"bla"' tags to define which YANG-fields map on which struct field.

But it seems when sending the PUT request to the restconf server it cannot find the correct struct fields, so it ends up ignoring the whole PUT request.

— Reply to this email directly, view it on GitHub https://github.com/freeconf/yang/issues/67#issuecomment-1642037957, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7V2E2KQV7LZJTTT4S3XQ7KXXANCNFSM6AAAAAA2PPAKHA . You are receiving this because you commented.Message ID: @.***>

HRogge commented 1 year ago

I have tried to build a small selfcontained demo application and uploaded it to https://github.com/HRogge/gorestconfdemo

I am using the curl commands described in curl.txt to access the module... GET access works but I cannot get a PUT command working.

dhubler commented 1 year ago

I did my best to tease out this issue into an isolated unit test and I could not reproduce the issue.

Can you review this and see if it matches your use case? https://github.com/freeconf/yang/blob/master/nodeutil/reflect_test.go#L598

HRogge commented 1 year ago

I found the problem in my code after our private discussion... I am still not convinced that the restriction to "struct only by pointer" (in case of container in container) is completely necessary, but I will have to look into this more in the future.

If I find a way to get rid of the constraints I will open a new issue.