Skip to content

[OFREP] Should variant and reason tolerate what the spec makes optional? #418

Description

@aepfli

Two questions about the same three lines in ofrep/__init__.py, both about optional fields being indexed unconditionally. Asking rather than reporting, in case either is a deliberate strictness choice.

value = data.get("value", default_value)      # tolerant
_typecheck_flag_value(value, flag_type)

return FlagResolutionDetails(
    value=value,
    reason=Reason[data["reason"]],            # (2)
    variant=data["variant"],                  # (1)
    flag_metadata=data.get("metadata", {}),
)

value is read with a default; the two below it are not. That asymmetry is what makes me think the others might be oversights rather than intent.

1. variant is optional, but indexed

types.md types it variant (string, optional), and OFREP's evaluationSuccess requires only key and reason.

Observed against flagd-testbed:v3.8.0:

$ curl -s -X POST localhost:8016/ofrep/v1/evaluate/flags/disabled-boolean-flag -d '{"context":{}}'
{"key":"disabled-boolean-flag","reason":"DISABLED","metadata":{}}     HTTP 200

KeyError: 'variant', surfacing to the application as GENERAL. Note value is already handled correctly for this response — OFREP's codeDefaultFlag schema omits value on purpose ("The provider must use the code default value when processing this response"), and data.get("value", default_value) does exactly that. So the response is understood apart from the variant.

This is not specific to disabled flags: any response omitting variant would do it.

Question: should this be data.get("variant"), or is there a reason to require the field?

2. Reason[...] is an enum lookup by name

Reason[data["reason"]] raises KeyError for any reason outside the nine members of Python's Reason enum. But Requirement 2.2.5 permits a provider to populate reason with one of the listed values "or some other string indicating the semantic reason for the returned flag value", and types.md adds:

The reason should not be limited to the reasons enumerated above. It can be any of the pre-defined reasons, or any string value.

The Python SDK models this correctly — FlagEvaluationDetails.reason is typed str | Reason | None. So an OFREP server returning a vendor-specific reason string appears to crash this provider on a response the specification explicitly allows.

I have not observed this one in the wild — flagd only returns standard reasons — so it is inference from the code, not a reproduction. Flagging it because it is the same line and the same class of thing.

Question: should the reason fall back to the raw string when it is not a known member, rather than raising?

Context

Found while building the cross-language provider conformance suite proposed in open-feature/spec#417. A new gated @disabled-flags capability exposed (1); (2) came from reading the surrounding lines. For calibration, the Go and JavaScript OFREP providers pass the same scenarios, so this does not look like a limitation of OFREP.

Happy to be told either of these is intended.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    questionFurther information is requested

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions