Skip to content

fix: provider exceptions crash the caller#97

Draft
NeaguGeorgiana23 wants to merge 1 commit into
mainfrom
fix_provider_exeptions
Draft

fix: provider exceptions crash the caller#97
NeaguGeorgiana23 wants to merge 1 commit into
mainfrom
fix_provider_exeptions

Conversation

@NeaguGeorgiana23
Copy link
Copy Markdown
Contributor

This PR

Resolves the issue where feature provider errors could propagate and crash the caller during flag evaluation.

To satisfy Requirement 1.4.10 while strictly adhering to the Google C++ Style Guide's zero-exception rules, this PR refactors the evaluation boundary to use compile-time contractual safety:

  • Updates the FeatureProvider evaluation interface to return absl::StatusOr.
  • Updates the client evaluation logic to gracefully handle failure statuses. If a provider returns an error status, the SDK safely intercepts it and returns the default value populated with a general error code.
  • Prevents abnormal termination

Related Issues

Fixes #69

Follow-up Tasks

  • Address remaining minor bugs from the Specification Compliance tracking issue.

Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the FeatureProvider interface and its implementations, including InMemoryProvider and NoopProvider, to return absl::StatusOr<std::unique_ptr<...>> for all flag evaluation methods. This change standardizes error reporting and explicitly discourages the use of C++ exceptions. Corresponding updates were made to the ClientAPI, test suites, and mocks to handle the new return types. Feedback suggests adding a null check for the unique_ptr within the absl::StatusOr in openfeature/client_api.h to prevent potential crashes if a provider returns an OK status with a null pointer.

Comment thread openfeature/client_api.h
Comment on lines +133 to +138
if (!result.ok()) {
return std::make_unique<ResolutionDetailsType>(
default_value, Reason::kError, std::nullopt, FlagMetadata(),
ErrorCode::kGeneral, std::string(result.status().message()));
}
return std::move(*result);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While checking result.ok() is necessary to handle explicit provider errors, it is also important to ensure that the provider did not return a nullptr inside the absl::StatusOr. If a provider implementation returns absl::OkStatus() but a null unique_ptr, the subsequent dereference in the caller (e.g., GetBooleanValue) will cause a crash. Given the PR's objective to prevent abnormal termination, adding a null check here provides an essential layer of safety.

  if (!result.ok() || *result == nullptr) {
    return std::make_unique<ResolutionDetailsType>(
        default_value, Reason::kError, std::nullopt, FlagMetadata(),
        ErrorCode::kGeneral,
        result.ok() ? "Provider returned null resolution details"
                    : std::string(result.status().message()));
  }
  return std::move(*result);

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specification Compliance Review

1 participant