open-feature / open-feature/python-sdk-contrib

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

Open
#418 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

question
Dominant language
Python
Stars
27
Forks
33
Avg merge
5h 7m
Merged PRs (30d)
9

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in ofrep/init.py at the FlagResolutionDetails construction and read the surrounding response-mapping code. Check the optional-field definitions in types.md and the referenced OFREP provider requirements. Done means responses without variant and responses with non-standard reason strings are handled without an unintended KeyError, with coverage for both cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.