search

openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
285 stars 107 forks source link

Change internal JSON Generation for OrderedMaps to be backwards-compatible #894

Closed wenovus closed 1 year ago

wenovus commented 1 year ago

I did not consider JSON internal format for OrderedMaps and was generating it the same way as RFC7951.

However, Internal JSON format for YANG lists uses a JSON object/map type instead of JSON list to represent a map.

Doing this destroys the ordering and defeats the purpose of ordered-by user lists but it's more important to keep backwards-compatibility for this legacy JSON format.

Also refactor code.

coveralls commented 1 year ago

Coverage Status

coverage: 89.649% (-0.01%) from 89.663% when pulling 208e7621b6f514ec16dc383f002afd1fc464ba38 on ordered-map-json-fix into db25a8cee822541a525dfc90f9084e22eef1eb42 on master.

DanG100 commented 1 year ago

should this behavior be controlled by a flag?

wenovus commented 1 year ago

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

DanG100 commented 1 year ago

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

Should there be a TODO to remove this internal JSON? especially if it's not generated "incorrect" info

wenovus commented 1 year ago

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

Should there be a TODO to remove this internal JSON? especially if it's not generated "incorrect" info

I think this might be reasonable. @robshakir do you think it's reasonable to mark as "deprecated" the ygot.Internal JSONFormat? The code comments mention that it is the format used by pyangbind so I'm not positive whether it should be marked as such, but it seems otherwise RFC7951 is becoming the default option rather than the internal format:

 const (
        // Internal is the custom JSON format that is output by the validation library, and
-       // by pyangbind. It is loosely specified - but is the default used by generator developers.
+       // by pyangbind. It is loosely specified - but is used by some generator developers.
+       //
+       // Deprecated: Prefer RFC7951 to preserve list ordering, among other
+       // advantages.
        Internal JSONFormat = iota
wenovus commented 1 year ago

I'll merge this now, but depending on Rob's comments I might add a deprecation notice.