Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use union type with empty type to enable "[null]" correct answer in case a mandatory data is not available #99

Closed
EstherLerouzic opened this issue Dec 3, 2021 · 9 comments · Fixed by #104
Assignees
Labels
agreed Resolution has been agreed: need for a PR to be merged before closing the issue draft text proposal exists I-D text proposal available IETF-113 Issue to be closed with PR for IETF-113

Comments

@EstherLerouzic
Copy link
Collaborator

If mandatory data are not present in the topology instance (for example due to a not responding server which makes the value unavailable), a yang-driven sofware will probably generate an "inconsitent" value based on the implementer or the attribute. eg fiber length is mandatory, and if not present the instance may contain an inconsistant data (eg -99999.99 or 0.00, or -1234 or ....) to satisfy the mandatoty requirement still conveying the information that data is not there.
In order to avoid these unspecified behaviour, I propose to explicitly use the empty type, so that if a mandatory data is not found the response should be of the empty type eg "[null]" for json case [RFC7951].
This means that all our types should be union with the empty type.

@EstherLerouzic
Copy link
Collaborator Author

To complete this issue, using empty type may also be useful for optional parameters when they are required for use cases.

For example, let's consider an amplifier element to represent a Raman pump. In this case tilt-target is not applicable. so in the yang tilt target can be put as optional to cover this use case. Then extracting an EDFA amplifier information with tilt-target = [null] has not the same meaning as not reporting the attribute at all: in one case it means that the value was required but not found (wrongly reported, issues with the server..., so needs some corrective action), in the other case, it means that this attribute is not applicable, so no error in the data or during the extraction.

@sergiobelotti
Copy link
Collaborator

AP @sergio,Italo and Esther: to provide text clarification about the usage of "empty" type for optional" attribute

@sergiobelotti
Copy link
Collaborator

sergiobelotti commented Jan 21, 2022

AP @sergio,Italo and Esther: to provide text clarification about the usage of "empty" type for optional" attribute

This is the text proposed for AP above:

"If an optional attribute is not applicable to an entity it MUST be omitted (not be present in the datastore).
If an optional/mandatory attribute is applicable but its value is unknown, it MUST be reported using the "empty" type. "

The proposal would be to add this text clarification in the description field of the YANG module.

@ju7ien
Copy link
Collaborator

ju7ien commented Jan 21, 2022

The following sentences seem easy to agree on:
"If an optional attribute is not applicable to an entity it MUST be omitted (not be present in the datastore).
If a the value of a mandatory attribute is unknown, it MUST be reported using the "empty" type."

I'm less sure about the "optional but applicable" case. It suggests it's a conditional attribute while the module doesn't express the associated criteria. Besides, there could be some situation where, for example, 2 among 3 attributes are enough: why would we mandate an implementation to include the 3rd one as null whereas there's no "missing value" to express?

How about: "If an optional attribute is applicable but its value is unknown, it SHOULD be reported using the "empty" type."?

@sergiobelotti
Copy link
Collaborator

sergiobelotti commented Jan 24, 2022

The following sentences seem easy to agree on: "If an optional attribute is not applicable to an entity it MUST be omitted (not be present in the datastore). If a the value of a mandatory attribute is unknown, it MUST be reported using the "empty" type."

I'm less sure about the "optional but applicable" case. It suggests it's a conditional attribute while the module doesn't express the associated criteria. Besides, there could be some situation where, for example, 2 among 3 attributes are enough: why would we mandate an implementation to include the 3rd one as null whereas there's no "missing value" to express?

How about: "If an optional attribute is applicable but its value is unknown, it SHOULD be reported using the "empty" type."?

@ju7ien : I would be fine with your modified text. @EstherLerouzic @italobusi @dieterbeller : what is your opinion?

@EstherLerouzic
Copy link
Collaborator Author

OK for me!

@dieterbeller
Copy link
Member

OK for me too

@italobusi
Copy link
Member

The following sentences seem easy to agree on: "If an optional attribute is not applicable to an entity it MUST be omitted (not be present in the datastore). If a the value of a mandatory attribute is unknown, it MUST be reported using the "empty" type."

I'm less sure about the "optional but applicable" case. It suggests it's a conditional attribute while the module doesn't express the associated criteria. Besides, there could be some situation where, for example, 2 among 3 attributes are enough: why would we mandate an implementation to include the 3rd one as null whereas there's no "missing value" to express?

How about: "If an optional attribute is applicable but its value is unknown, it SHOULD be reported using the "empty" type."?

I think that the SHOULD will bring back the ambiguity between the attribute not being present and being reported using the "empty" type

IMHO, there is no value to add the "empty" type in the YANG model if we allow an implementation to decide whether to omit the attribute or to report it using the "empty" type

In the example above, if 2 among 3 attributes are enough, IMHO the third attribute MUST be omitted. I think that the third attribute can be considered as "not applicable". Maybe we can find a better wording for "not applicable" ...

@sergiobelotti sergiobelotti added agreed Resolution has been agreed: need for a PR to be merged before closing the issue draft text update Requires an update of the I-D text labels Jan 25, 2022
@sergiobelotti
Copy link
Collaborator

sergiobelotti commented Jan 25, 2022

This is the final text agreed during the meeting of January 25 th:
"If the value of a mandatory attribute is unknown, it MUST be reported using the "empty" type.
If an optional attribute is applicable but its value is unknown it MUST be reported using the "empty" type.
If an optional attribute is not applicable to an entity it MUST be omitted (not be present in the datastore)."
This text will be added in the description of the module in the YANG .

@sergiobelotti sergiobelotti added draft text proposal exists I-D text proposal available IETF-113 Issue to be closed with PR for IETF-113 and removed draft text update Requires an update of the I-D text labels Jan 27, 2022
italobusi added a commit to ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis that referenced this issue Mar 2, 2022
Fix #32 :
- added bitrate to the common-explicit-mode grouping

Support resolution of ietf-ccamp-wg/draft-ietf-ccamp-optical-impairment-topology-yang#99:
- added the following typedef
  - snr-or-null
  - decimal-2-digits
  - decimal-2-digits-or-null
  - power-in-db
  - power-in-db-or-null
  - power-in-dbm
  - power-in-dbm-or-null
- Updated types with union with type empty when applicable

Co-Authored-By: sergio belotti <sergio.belotti@nokia.com>
italobusi added a commit that referenced this issue Mar 2, 2022
Fix #85
- added total-output-power to the amplifier-params grouping
Fix #99 :
- Updated types with union with type empty when applicable
Fix #100
- via PR ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis#36
Fix #102 :
- added raman-direction to the amplifier-params grouping
- added raman-pump list to the amplifier-params grouping

Co-Authored-By: sergio belotti <sergio.belotti@nokia.com>
italobusi added a commit to ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis that referenced this issue Mar 7, 2022
* Initial proposal for RFC9093-bis

Initial proposal from RFC9093-bis text merging text from RFC9093 and draft-ietf-ccamp-layer0-types-ext-01

Created new revision of ietf-layer0-types.yang adding the content of ietf-layer0-type-ext.yang to the first version of ietf-layer0-types.yang

Text edited using kramdown

Fix #32 :
- added bitrate to the common-explicit-mode grouping

Support resolution of ietf-ccamp-wg/draft-ietf-ccamp-optical-impairment-topology-yang#99:
- added the following typedef
  - snr-or-null
  - decimal-2-digits
  - decimal-2-digits-or-null
  - power-in-db
  - power-in-db-or-null
  - power-in-dbm
  - power-in-dbm-or-null
- Updated types with union with type empty when applicable

Fix #35
- added grouping l0-path-constraints
- added grouping l0-path-properties

Fix #37 :
- moved to YANG 1.1

Added placeholder for describing the changes from RFC 9093 (new issue #40 )

Co-Authored-By: sergio belotti <sergio.belotti@nokia.com>
Co-Authored-By: Dieter Beller <Dieter.Beller@nokia.com>
italobusi added a commit that referenced this issue Mar 7, 2022
Updated YANG: aligned with latest version of layer0-types version 2

Added description related to issue #99

Co-Authored-By: sergio belotti <sergio.belotti@nokia.com>
italobusi added a commit that referenced this issue Mar 7, 2022
* Updated YANG model for IETF 113

Fix #85
- added total-output-power to the amplifier-params grouping

Fix #99 :
- Updated types with union with type empty when applicable
- Added description about the empty types

Fix #100
- via PR ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis#36

Fix #102 :
- added raman-direction to the amplifier-params grouping
- added raman-pump list to the amplifier-params grouping

Fix #103:
- YANG aligned with PR ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis#36

Co-authored-by: sergio belotti <sergio.belotti@nokia.com>
Co-authored-by: Dieter Beller <Dieter.Beller@nokia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agreed Resolution has been agreed: need for a PR to be merged before closing the issue draft text proposal exists I-D text proposal available IETF-113 Issue to be closed with PR for IETF-113
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants