fix: no-op when API key is blank or supervisor unavailable#124
fix: no-op when API key is blank or supervisor unavailable#124marandaneto wants to merge 7 commits into
Conversation
posthog-elixir Compliance ReportDate: 2026-05-31 14:05:47 UTC
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 608ms |
| Format Validation.Event Has Uuid | ✅ | 610ms |
| Format Validation.Event Has Lib Properties | ✅ | 609ms |
| Format Validation.Distinct Id Is String | ✅ | 609ms |
| Format Validation.Token Is Present | ✅ | 609ms |
| Format Validation.Custom Properties Preserved | ✅ | 609ms |
| Format Validation.Event Has Timestamp | ✅ | 609ms |
| Retry Behavior.Retries On 503 | ✅ | 5614ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2612ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2611ms |
| Retry Behavior.Respects Retry After Header | ✅ | 5614ms |
| Retry Behavior.Implements Backoff | ✅ | 15625ms |
| Retry Behavior.Retries On 500 | ✅ | 5610ms |
| Retry Behavior.Retries On 502 | ✅ | 5615ms |
| Retry Behavior.Retries On 504 | ✅ | 5615ms |
| Retry Behavior.Max Retries Respected | ✅ | 15611ms |
| Deduplication.Generates Unique Uuids | ✅ | 620ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 5614ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10617ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 5616ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 614ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 612ms |
| Compression.Sends Gzip When Enabled | ✅ | 609ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 609ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 607ms |
| Batch Format.Multiple Events Batched Together | ✅ | 614ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2611ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2612ms |
| Error Handling.Retries On 408 | ✅ | 5615ms |
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 8ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 6ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 6ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 6ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 6ms |
| Request Payload.Groups Round Trip | ✅ | 6ms |
| Request Payload.Groups Default To Empty Object | ✅ | 6ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ✅ | 5ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 6ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 6ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 6ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 3ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 607ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 10ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 6ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 608ms |
Failures
request_payload.disable_geoip_omitted_defaults_to_false
Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['groups', 'api_key', 'distinct_id', 'flag_keys_to_evaluate', 'group_properties', 'person_properties']
request_lifecycle.mock_response_value_is_returned_to_caller
Last action result missing field 'value'. Keys: ['error', 'success']
side_effect_events.get_feature_flag_captures_feature_flag_called_event
Expected 1 events with name '$feature_flag_called', got 0
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
test/posthog/config_test.exs:63-134
**Prefer parameterised tests for the four blank-key cases**
The four tests (`missing`, `nil`, `empty`, `blank after trimming whitespace`) exercise exactly the same assertions with only the input value changing. Per the team's convention, these should be a single parameterised test. The same applies to the two `read!` tests in `application_config_test.exs` (`missing` vs `nil`). A for-comprehension over a list of `{label, input}` pairs keeps the intent clear and removes the duplication.
### Issue 2 of 2
lib/posthog/config.ex:258-266
The third `then/2` block injects `api_host: @default_api_host` when `api_key` is blank and `api_host` is absent from the keyword list. However, the NimbleOptions schema already declares `default: @default_api_host` for `api_host`, so NimbleOptions itself will supply that default during validation — the explicit injection is redundant and adds a code path that has no observable effect.
```suggestion
end
```
Reviews (1): Last reviewed commit: "Disable SDK when API key is blank" | Re-trigger Greptile |
|
can't approve, but LGTM |
|
One last comment before this is merged, if I may. Comparing this solution to #114 I can't brush off the feeling, that something is off. One concern we had is that introducing This PR didn't introduce new public configuration options, but it did introduce a hidden one: Finally, when we configure the SDK to drop events in dev (
which is completely non-actionable, since the SDK is doing exactly what we want it to and there is no other way to configure it to do that! Perhaps we could reconsider adding explicit |
|
@martosaur if you dont want to capture events in test mode, just dont init the SDK, simple as that what is the problem with this approach? some other SDKs actually expose an some other SDKs expose an we dont need yet another flag that does something similar, i'd just go with one of those. this PR addresses an issue that crashes the SDK at runtime if no api key is provided, and the enabled flag is private, so this can be changed at any given time. |
|
|
Correct, you can achieve the same with the |
|
Reviews (2): Last reviewed commit: "fix: no-op when supervisor is unavailabl..." | Re-trigger Greptile |
💡 Motivation and Context
Applications should be able to run safely when PostHog is disabled, missing a usable API key, or the PostHog supervisor is unavailable. Public SDK calls should no-op or return empty feature flag defaults instead of crashing the host process.
Changes:
💚 How did you test it?
mix test test/posthog/uninitialized_test.exs test/posthog/supervisor_test.exs test/posthog/feature_flags_test.exs📝 Checklist
If releasing new changes
sampo addto generate a changeset file