Skip to content

fix: empty API key initialization#117

Merged
marandaneto merged 2 commits into
mainfrom
fix/noop-empty-api-key
May 28, 2026
Merged

fix: empty API key initialization#117
marandaneto merged 2 commits into
mainfrom
fix/noop-empty-api-key

Conversation

@marandaneto
Copy link
Copy Markdown
Member

@marandaneto marandaneto commented May 28, 2026

💡 Motivation and Context

Initializing the Rust SDK without a project API key, or with a whitespace-only key, should be safe and should not send requests with an empty key.

This change disables the client when the API key is missing or blank so capture, batch capture, and feature flag calls become no-ops instead of making outbound requests. It also keeps option sanitization centralized in the builder and consolidates the noop API-key coverage across both missing and whitespace-only cases.

💚 How did you test it?

  • cargo test
  • cargo test --no-default-features

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran sampo add to generate a changeset file

@marandaneto marandaneto requested a review from a team as a code owner May 28, 2026 14:25
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 28, 2026

Comments Outside Diff (2)

  1. src/client/mod.rs, line 147-167 (link)

    P2 Double sanitize in From convenience impls

    Both From<&str> and From<(&str, &str)> now call .build() — which already invokes sanitize() internally — and then chain .with_endpoint_manager(), which is just an alias for sanitize() again. The host normalisation and EndpointManager construction are performed twice on every Client::from("my-key") call. The .with_endpoint_manager() call can be dropped from both impls.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/client/mod.rs
    Line: 147-167
    
    Comment:
    **Double sanitize in `From` convenience impls**
    
    Both `From<&str>` and `From<(&str, &str)>` now call `.build()` — which already invokes `sanitize()` internally — and then chain `.with_endpoint_manager()`, which is just an alias for `sanitize()` again. The host normalisation and `EndpointManager` construction are performed twice on every `Client::from("my-key")` call. The `.with_endpoint_manager()` call can be dropped from both impls.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. src/client/mod.rs, line 176-201 (link)

    P2 Redundant .sanitize() calls in existing unit tests

    trims_whitespace_sensitive_options and defaults_blank_host_after_trimming_whitespace both call .build().unwrap().sanitize(). Since build() now calls sanitize() internally as part of this PR, the trailing .sanitize() is superfluous and should be removed.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/client/mod.rs
    Line: 176-201
    
    Comment:
    **Redundant `.sanitize()` calls in existing unit tests**
    
    `trims_whitespace_sensitive_options` and `defaults_blank_host_after_trimming_whitespace` both call `.build().unwrap().sanitize()`. Since `build()` now calls `sanitize()` internally as part of this PR, the trailing `.sanitize()` is superfluous 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 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/client/mod.rs:147-167
**Double sanitize in `From` convenience impls**

Both `From<&str>` and `From<(&str, &str)>` now call `.build()` — which already invokes `sanitize()` internally — and then chain `.with_endpoint_manager()`, which is just an alias for `sanitize()` again. The host normalisation and `EndpointManager` construction are performed twice on every `Client::from("my-key")` call. The `.with_endpoint_manager()` call can be dropped from both impls.

### Issue 2 of 3
src/client/mod.rs:176-201
**Redundant `.sanitize()` calls in existing unit tests**

`trims_whitespace_sensitive_options` and `defaults_blank_host_after_trimming_whitespace` both call `.build().unwrap().sanitize()`. Since `build()` now calls `sanitize()` internally as part of this PR, the trailing `.sanitize()` is superfluous and should be removed.

### Issue 3 of 3
tests/test_async.rs:407-504
**Non-parameterised tests for the two noop cases**

Per the team's preference for parameterised tests, `test_client_with_missing_api_key_is_noop` and `test_client_with_whitespace_api_key_is_noop` (and their blocking counterparts) test the same behaviour with only the client construction differing. Consolidating into a single parameterised test (e.g. using `rstest`) would remove the duplicated setup and assertion logic. The whitespace-key test also currently only checks `get_feature_flags`, while the missing-key test checks all methods — parameterisation would naturally close that gap.

Reviews (1): Last reviewed commit: "Fix empty api key initialization" | Re-trigger Greptile

Comment thread tests/test_async.rs
@marandaneto marandaneto changed the title Fix empty API key initialization fix: empty API key initialization May 28, 2026
@marandaneto
Copy link
Copy Markdown
Member Author

Addressed the Greptile feedback in 49a81d9:

  • Removed the redundant sanitize calls from the From convenience impls.
  • Removed redundant .sanitize() calls from the existing client options tests.
  • Consolidated the empty API key noop tests so missing and whitespace-only keys share the same assertions for async and blocking clients.

Verified with cargo test and cargo test --no-default-features.

@marandaneto marandaneto merged commit deb361b into main May 28, 2026
10 checks passed
@marandaneto marandaneto deleted the fix/noop-empty-api-key branch May 28, 2026 14:43
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.

2 participants