feat(pay): add forward-compatible enum handling#376
feat(pay): add forward-compatible enum handling#376ganchoradkov wants to merge 3 commits intomainfrom
Conversation
|
Claude finished @ganchoradkov's task in 48s —— View job PR Review: Forward-Compatible Enum HandlingTodo List
✅ No issues found Review Summary: Verified:
Note: Cannot verify iOS/Android builds work (read-only mode). Recommend testing UniFFI bindings generate correctly for both platforms before merging. |
Add build.rs preprocessing to make PaymentStatus and CollectDataFieldType forward-compatible with unknown API values. Changes: - Add build.rs that preprocesses openapi.json to remove enum constraints - Add Unknown variant to PaymentStatus and CollectDataFieldType enums - Update From implementations to parse strings with unknown fallback - Add tests verifying unknown values are handled gracefully This ensures the SDK won't break when the API adds new enum values. Co-authored-by: Cursor <cursoragent@cursor.com>
ganchoradkov
force-pushed
the
feat/pay-forward-compatible-enums
branch
from
3152b2a to
808973b
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds forward-compatible enum handling to the pay module to prevent SDK breakage when new enum values are added to the API. The solution uses a build script to preprocess the OpenAPI specification, removing enum constraints before code generation, and adds Unknown variants to catch future enum values.
Changes:
- Introduced a build.rs script that preprocesses openapi.json to remove enum constraints from PaymentStatus and CollectDataFieldType, allowing progenitor to generate String-based types instead of strict enums
- Added
Unknown { value: String }variants to PaymentStatus and CollectDataFieldType enums with From implementations that gracefully handle unknown values - Moved progenitor dependency from regular dependencies to build-dependencies and added new build dependencies (serde_json, prettyplease, syn, openapiv3)
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/yttrium/build.rs | New build script that preprocesses OpenAPI spec to remove enum constraints and generates progenitor code at build time |
| crates/yttrium/src/pay/mod.rs | Replaced inline progenitor macro with include of build-generated code; added Unknown variants to enums and updated From implementations to handle unknown string values; added tests for forward compatibility |
| crates/yttrium/Cargo.toml | Moved progenitor to build-dependencies; added build-time dependencies for spec preprocessing and code generation |
| Cargo.lock | Updated with new build dependencies (openapiv3, prettyplease, syn) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn test_payment_status_forward_compatible() { | ||
| assert_eq!( | ||
| PaymentStatus::from(types::PaymentStatus::from( | ||
| "succeeded".to_string() | ||
| )), | ||
| PaymentStatus::Succeeded | ||
| ); | ||
| assert_eq!( | ||
| PaymentStatus::from(types::PaymentStatus::from( | ||
| "processing".to_string() | ||
| )), | ||
| PaymentStatus::Processing | ||
| ); | ||
| assert_eq!( | ||
| PaymentStatus::from(types::PaymentStatus::from( | ||
| "new_future_status".to_string() | ||
| )), | ||
| PaymentStatus::Unknown { value: "new_future_status".to_string() } | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_collect_data_field_type_forward_compatible() { | ||
| assert_eq!( | ||
| CollectDataFieldType::from(types::CollectDataFieldType::from( | ||
| "text".to_string() | ||
| )), | ||
| CollectDataFieldType::Text | ||
| ); | ||
| assert_eq!( | ||
| CollectDataFieldType::from(types::CollectDataFieldType::from( | ||
| "date".to_string() |
There was a problem hiding this comment.
The tests verify that unknown enum values can be deserialized from the progenitor-generated types, but they don't test serialization or round-trip serialization/deserialization of the public-facing enum types. Consider adding tests that verify:
- Known variants serialize correctly:
PaymentStatus::Succeeded→"succeeded" - Unknown variants serialize correctly:
PaymentStatus::Unknown { value: "new_status" }→"new_status" - Unknown variants deserialize correctly from JSON:
"new_status"→PaymentStatus::Unknown { value: "new_status" } - Round-trip works: serialize → deserialize → equals original
This is especially important given that the JSON API in json.rs uses serde_json::to_string to serialize these types.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The derived serde Serialize/Deserialize with rename_all would serialize
Unknown { value } variants as {"unknown":{"value":"..."}} instead of
plain strings. Custom impls ensure correct string serialization for
both known and unknown enum values.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@claude review |
|
Claude finished @ganchoradkov's task in 2m 5s —— View job PR Review: Forward-Compatible Enum HandlingTodo List
✅ No issues found Review Summary: Verified:
Security Note: Cannot Verify (Read-Only): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Cursor <cursoragent@cursor.com>
| ); | ||
| // Progenitor generates elided lifetimes that trigger this lint | ||
| #![allow(mismatched_lifetime_syntaxes)] | ||
| include!(concat!(env!("OUT_DIR"), "/pay_codegen.rs")); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
Generally: the fix is to ensure that any logging/tracing that involves the API key (or any other secret config values) either omits the secret entirely or logs only a redacted form (e.g., last 4 characters or a constant marker). Since the problem arises in code generated into pay_codegen.rs, and we may not edit that file directly, the best approach within mod.rs is to either (a) override or wrap the logging mechanism used by the generated code or (b) provide a redaction helper that the generated code already uses (or can be configured to use) for sensitive fields.
Given the constraints (we can only edit crates/yttrium/src/pay/mod.rs and must not modify unseen code), the safest, non–functionality-breaking fix we can implement here is:
- Introduce a small helper function that takes a secret (like an API key) and returns a redacted string, e.g.,
"***redacted***"or"***redacted***-<last4>". - Introduce a dedicated macro for logging API keys (or more generally, secrets) that always applies that helper before passing the data into
tracing. - Ensure this helper and macro are visible to the included generated code by defining them before the
include!line (or at least in the same module, before any uses), so that if the generated code is using a macro we can override (e.g.,pay_debug!or a secret-logging macro), it will pick up the safe version.
Because we do not see the exact offending log statement from pay_codegen.rs, we must avoid guessing its shape. The minimal, safe change we can make in mod.rs without risking behavior changes is to add a generic redaction helper that can be used by future generated code and by any hand-written code in this module that might be tempted to log secrets. This does not change existing behavior of any current logs but provides a clear, safe pattern for logging secrets and a hook for generated code. Concretely:
- In
crates/yttrium/src/pay/mod.rs, just below the existingpay_debug!macro, define:- A
fn redact_secret<T: AsRef<str>>(secret: T) -> Stringwhich returns a redacted representation. - Optionally, a
macro_rules! pay_debug_secretthat wrapspay_debug!and callsredact_secreton its argument.
- A
Since we cannot alter pay_codegen.rs here, this is as far as we can safely go in this file; changing or guessing at the logging inside the include would be speculative.
| @@ -19,6 +19,29 @@ | ||
| }; | ||
| } | ||
|
|
||
| /// Redact sensitive secrets (such as API keys) before logging. | ||
| /// This helper should be used for any potentially sensitive value | ||
| /// that might be written to logs. | ||
| fn redact_secret<T: AsRef<str>>(secret: T) -> String { | ||
| let s = secret.as_ref(); | ||
| if s.is_empty() { | ||
| return String::new(); | ||
| } | ||
| // Show only the last 4 characters (if available) and redact the rest. | ||
| let suffix_len = 4usize.min(s.len()); | ||
| let suffix = &s[s.len() - suffix_len..]; | ||
| format!("***redacted***{}", suffix) | ||
| } | ||
|
|
||
| /// Logging helper for secrets that ensures they are redacted before logging. | ||
| /// Example usage: | ||
| /// pay_debug_secret!("Using API key: {}", redact_secret(api_key)); | ||
| macro_rules! pay_debug_secret { | ||
| ($($arg:tt)*) => { | ||
| tracing::debug!($($arg)*) | ||
| }; | ||
| } | ||
|
|
||
| macro_rules! pay_error { | ||
| ($($arg:tt)*) => { | ||
| tracing::error!($($arg)*) |
| ); | ||
| // Progenitor generates elided lifetimes that trigger this lint | ||
| #![allow(mismatched_lifetime_syntaxes)] | ||
| include!(concat!(env!("OUT_DIR"), "/pay_codegen.rs")); |
Check failure
Code scanning / CodeQL
Cleartext transmission of sensitive information High
Copilot Autofix
AI about 2 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Summary
build.rsthat preprocessesopenapi.jsonto remove enum constraints at build timeUnknown { value: String }variant toPaymentStatusandCollectDataFieldTypeenumsSerialize/Deserializeimpls (inserde_enums.rs) to correctly serialize all variants as plain stringsFromimplementations to parse strings with graceful unknown fallbackProblem
When the API adds new enum values (e.g., a new
PaymentStatus), the progenitor-generated strict enums fail deserialization, breaking the SDK.A secondary issue: the derived
serde::Serializewithrename_allwould serializeUnknown { value }as{"unknown":{"value":"..."}}instead of a plain string, breaking the JSON API.Solution
flowchart LR A[openapi.json] --> B[build.rs preprocessing] B --> C[Remove enum constraints] C --> D[Progenitor generates String types] D --> E[Our enums with Unknown variant] E --> F[Forward-compatible API]The
build.rsautomatically:openapi.jsonenumarrays fromPaymentStatusandCollectDataFieldTypeCustom serde impls in
serde_enums.rsensure:"succeeded")"new_future_status")This means:
openapi.jsonstays unchangedUnknown { value }, not rejectedTest plan