Skip to content

fix: Disable client when project token is missing#189

Open
marandaneto wants to merge 1 commit intomainfrom
fix/noop-empty-project-token
Open

fix: Disable client when project token is missing#189
marandaneto wants to merge 1 commit intomainfrom
fix/noop-empty-project-token

Conversation

@marandaneto
Copy link
Copy Markdown
Member

@marandaneto marandaneto commented Apr 30, 2026

Summary

  • Normalize ProjectToken and legacy ProjectApiKey by trimming and treating empty/whitespace as missing
  • Add a PostHogOptions.Disabled flag (default false) and set it when token normalization determines the SDK cannot be initialized
  • No-op client calls when disabled instead of sending an empty api_key or throwing later
  • Log a warning when public client methods are called while the SDK is disabled
  • No-op public methods that require personal_api_key when it is not configured, including local-only feature flag evaluation and remote config payloads
  • Add focused coverage for missing/empty/whitespace ProjectToken and legacy ProjectApiKey, disabled-call logging, and personal_api_key-protected methods

Tests

  • dotnet test tests/UnitTests/UnitTests.csproj --framework net8.0 --filter "FullyQualifiedName~TheDisabledClient|FullyQualifiedName~ThePersonalApiKeyProtectedMethods|FullyQualifiedName~LogsWarningWhenPersonalApiKey|FullyQualifiedName~LogsErrorWhenProjectTokenIsMissing|FullyQualifiedName~LogsWarningWhenProjectApiKeyIsUsed"
  • dotnet test tests/UnitTests/UnitTests.csproj --framework net8.0 (fails in existing CaptureException tests: CaptureExceptionWithInvalidFilePathInStackFrame, CaptureExceptionCauseIOFailureEmptyContext)

Note: requested context files context.md and plan.md were not present in this checkout.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

posthog-dotnet Compliance Report

Date: 2026-04-30 11:37:09 UTC
Duration: 250ms

⚠️ Some Tests Failed

0/1 tests passed, 1 failed


Feature_Flags Tests

⚠️ 0/1 tests passed, 1 failed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 40ms

Failures

request_payload.request_with_person_properties_device_id

404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'

@marandaneto marandaneto force-pushed the fix/noop-empty-project-token branch 9 times, most recently from bed79fb to 1ea64e2 Compare April 30, 2026 11:13
@marandaneto marandaneto marked this pull request as ready for review April 30, 2026 11:14
@marandaneto marandaneto requested review from a team and haacked as code owners April 30, 2026 11:14
@marandaneto
Copy link
Copy Markdown
Member Author

@PostHog/team-client-libraries and @haacked i think this is what SDKs should do
can you double check so i can port this fix across SDKs? (i have a bunch of draft PRs already but some of them didnt address all stuff i found during this one)

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Comments Outside Diff (1)

  1. src/PostHog/PostHogClient.cs, line 757-761 (link)

    P1 Double warning log in LoadFeatureFlagsAsync

    Same issue as GetRemoteConfigPayloadAsync: IsPersonalApiKeyMissing already logs the generic "personal_api_key is not configured" warning before control reaches LogWarningPersonalApiKeyRequired inside the if-body. The old call is now redundant and should be removed.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/PostHog/PostHogClient.cs
    Line: 757-761
    
    Comment:
    **Double warning log in `LoadFeatureFlagsAsync`**
    
    Same issue as `GetRemoteConfigPayloadAsync`: `IsPersonalApiKeyMissing` already logs the generic "personal_api_key is not configured" warning before control reaches `LogWarningPersonalApiKeyRequired` inside the `if`-body. The old call is now redundant and should be removed.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/PostHog/PostHogClient.cs:553-556
**Double warning log emitted when `PersonalApiKey` is missing**

`IsPersonalApiKeyMissing` already emits `LogWarningPersonalApiKeyMissing` ("PostHog personal_api_key is not configured; {MethodName} is a no-op.") before this `if`-body executes. The call to `LogWarningPersonalApiKeyRequiredForRemoteConfigPayload` on line 555 then fires a second, redundant warning for the same condition. The same pattern exists in `LoadFeatureFlagsAsync` (line 759, `LogWarningPersonalApiKeyRequired`). The old method-specific log calls should be removed now that the unified `IsPersonalApiKeyMissing` helper handles the logging, otherwise every missing-key event produces two warnings.

```suggestion
        if (IsPersonalApiKeyMissing(nameof(GetRemoteConfigPayloadAsync)))
        {
            return null;
        }
```

### Issue 2 of 2
src/PostHog/PostHogClient.cs:757-761
**Double warning log in `LoadFeatureFlagsAsync`**

Same issue as `GetRemoteConfigPayloadAsync`: `IsPersonalApiKeyMissing` already logs the generic "personal_api_key is not configured" warning before control reaches `LogWarningPersonalApiKeyRequired` inside the `if`-body. The old call is now redundant and should be removed.

```suggestion
        if (IsPersonalApiKeyMissing(nameof(LoadFeatureFlagsAsync)))
        {
            return;
        }
```

Reviews (1): Last reviewed commit: "Disable client when project token is mis..." | Re-trigger Greptile

Comment thread src/PostHog/PostHogClient.cs
@marandaneto marandaneto force-pushed the fix/noop-empty-project-token branch from 1ea64e2 to caef5ff Compare April 30, 2026 11:32
@marandaneto marandaneto force-pushed the fix/noop-empty-project-token branch from caef5ff to e8527fc Compare April 30, 2026 11:36
@marandaneto marandaneto changed the title Disable client when project token is missing fix: Disable client when project token is missing Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant