[transferwebhooks] Code generation: update services and models#1699
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates several webhook models to support unknown enum values during deserialization and simplifies property assignments by removing redundant value accessors. However, the reviewer identified several critical issues: the new enum conversion logic introduces a risk of NullReferenceException by accessing properties on potentially null objects across multiple files. Additionally, the updated polymorphic deserialization logic in classes like BankAccountV3AccountIdentification is flagged for being inefficient and for logically contradicting the goal of supporting unknown enums. Feedback also suggests implementing more robust null handling when casting raw JSON strings to enum types to avoid creating invalid enum instances.
| if (value == TypeEnum.AuLocal) | ||
| return "auLocal"; | ||
|
|
||
| return null; | ||
| return value.Value; |
There was a problem hiding this comment.
The implicit conversion to string? will throw a NullReferenceException if value is null. While the previous implementation safely returned null, the new code attempts to access value.Value without a null check. This is a critical issue as it can cause crashes during serialization or whenever an enum property is accessed as a string. This pattern is repeated across almost all model files updated in this PR.
if (value == TypeEnum.AuLocal)
return "auLocal";
return value?.Value;| if (value == AmountAdjustmentTypeEnum.ForexMarkup) | ||
| return "forexMarkup"; | ||
|
|
||
| return null; | ||
| return value.Value; |
There was a problem hiding this comment.
| if (value == CategoryEnum.TopUp) | ||
| return "topUp"; | ||
|
|
||
| return null; | ||
| return value.Value; |
There was a problem hiding this comment.
| if (aULocalAccountIdentification?.Type != null && AULocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)aULocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(aULocalAccountIdentification); | ||
|
|
||
| if (bRLocalAccountIdentification?.Type != null) | ||
| if (bRLocalAccountIdentification?.Type != null && BRLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)bRLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(bRLocalAccountIdentification); | ||
|
|
||
| if (cALocalAccountIdentification?.Type != null) | ||
| if (cALocalAccountIdentification?.Type != null && CALocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)cALocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(cALocalAccountIdentification); | ||
|
|
||
| if (cZLocalAccountIdentification?.Type != null) | ||
| if (cZLocalAccountIdentification?.Type != null && CZLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)cZLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(cZLocalAccountIdentification); | ||
|
|
||
| if (dKLocalAccountIdentification?.Type != null) | ||
| if (dKLocalAccountIdentification?.Type != null && DKLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)dKLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(dKLocalAccountIdentification); | ||
|
|
||
| if (hKLocalAccountIdentification?.Type != null) | ||
| if (hKLocalAccountIdentification?.Type != null && HKLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)hKLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(hKLocalAccountIdentification); | ||
|
|
||
| if (hULocalAccountIdentification?.Type != null) | ||
| if (hULocalAccountIdentification?.Type != null && HULocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)hULocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(hULocalAccountIdentification); | ||
|
|
||
| if (ibanAccountIdentification?.Type != null) | ||
| if (ibanAccountIdentification?.Type != null && IbanAccountIdentification.TypeEnum.FromStringOrDefault((string?)ibanAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(ibanAccountIdentification); | ||
|
|
||
| if (nOLocalAccountIdentification?.Type != null) | ||
| if (nOLocalAccountIdentification?.Type != null && NOLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)nOLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(nOLocalAccountIdentification); | ||
|
|
||
| if (nZLocalAccountIdentification?.Type != null) | ||
| if (nZLocalAccountIdentification?.Type != null && NZLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)nZLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(nZLocalAccountIdentification); | ||
|
|
||
| if (numberAndBicAccountIdentification?.Type != null) | ||
| if (numberAndBicAccountIdentification?.Type != null && NumberAndBicAccountIdentification.TypeEnum.FromStringOrDefault((string?)numberAndBicAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(numberAndBicAccountIdentification); | ||
|
|
||
| if (pLLocalAccountIdentification?.Type != null) | ||
| if (pLLocalAccountIdentification?.Type != null && PLLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)pLLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(pLLocalAccountIdentification); | ||
|
|
||
| if (sELocalAccountIdentification?.Type != null) | ||
| if (sELocalAccountIdentification?.Type != null && SELocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)sELocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(sELocalAccountIdentification); | ||
|
|
||
| if (sGLocalAccountIdentification?.Type != null) | ||
| if (sGLocalAccountIdentification?.Type != null && SGLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)sGLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(sGLocalAccountIdentification); | ||
|
|
||
| if (uKLocalAccountIdentification?.Type != null) | ||
| if (uKLocalAccountIdentification?.Type != null && UKLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)uKLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(uKLocalAccountIdentification); | ||
|
|
||
| if (uSLocalAccountIdentification?.Type != null) | ||
| if (uSLocalAccountIdentification?.Type != null && USLocalAccountIdentification.TypeEnum.FromStringOrDefault((string?)uSLocalAccountIdentification.Type) != null) | ||
| return new BankAccountV3AccountIdentification(uSLocalAccountIdentification); |
There was a problem hiding this comment.
The polymorphic deserialization logic here has two significant issues:
- Efficiency: For every variant, it performs a null check, an implicit string conversion, and a
FromStringOrDefaultlookup. This is repeated up to 16 times per object. A more efficient approach would be to extract thetypeproperty from the JSON once and use it to select the correct model. - Logic Contradiction: The check
FromStringOrDefault(...) != nullensures that only known enum values are accepted for each variant. However, the individual models (likeAULocalAccountIdentification) were updated in this PR to support unknown enum values. This wrapper effectively negates that change for polymorphic types, as it will throw aJsonExceptionif thetypestring is not recognized by any variant, even if one of the models could have handled it as an unknown enum value.
| case "type": | ||
| string? typeRawValue = utf8JsonReader.GetString(); | ||
| type = new Option<AULocalAccountIdentification.TypeEnum?>(AULocalAccountIdentification.TypeEnum.FromStringOrDefault(typeRawValue)); | ||
| type = new Option<AULocalAccountIdentification.TypeEnum?>(AULocalAccountIdentification.TypeEnum.FromStringOrDefault(typeRawValue) ?? (AULocalAccountIdentification.TypeEnum)typeRawValue); |
There was a problem hiding this comment.
When typeRawValue is null (e.g., if the JSON property is explicitly set to null), the expression (AULocalAccountIdentification.TypeEnum)typeRawValue might create an 'empty' enum instance instead of returning a null reference, depending on how the explicit cast is implemented. It is safer to explicitly handle the null case to ensure the property remains null when the input is null.
type = new Option<AULocalAccountIdentification.TypeEnum?>(typeRawValue == null ? null : (AULocalAccountIdentification.TypeEnum.FromStringOrDefault(typeRawValue) ?? (AULocalAccountIdentification.TypeEnum)typeRawValue));
This PR contains the automated changes for the
transferwebhooksservice.The commit history of this PR reflects the
adyen-openapicommits that have been applied.