Fixes unmarshalling of YAML #3116
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves YAML unmarshalling by first parsing into a generic map to preserve key casing, then converting to JSON to leverage struct JSON tags, and adds a fallback for apiVersion if it wasn’t populated during the JSON unmarshal.
- Introduce a two-step YAML→map→JSON→struct unmarshalling flow
- Add fallback assignment for
application.ApiVersionfrom raw map - (Unintended) add a debug print of the
applicationstruct
Comments suppressed due to low confidence (1)
internal/flink/command_application_create.go:62
- Add unit tests for the new YAML unmarshalling path, including cases where
apiVersionis populated via the raw map fallback.
var raw map[string]interface{}
| default: | ||
| return errors.NewErrorWithSuggestions(fmt.Sprintf("unsupported file format: %s", ext), "Supported file formats are .json, .yaml, and .yml.") | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Print("application", application) |
There was a problem hiding this comment.
Remove this debug print or replace it with a proper logger call at the appropriate log level to avoid dumping raw structs to stdout in production.
| fmt.Print("application", application) | |
| log.Printf("Debug: application struct: %+v", application) |
Copilot uses AI. Check for mistakes.
| // First unmarshal into a generic map to preserve the case | ||
| var raw map[string]interface{} | ||
| if err = yaml.Unmarshal(data, &raw); err != nil { | ||
| return fmt.Errorf("failed to unmarshal YAML: %v", err) |
There was a problem hiding this comment.
Use fmt.Errorf("failed to unmarshal YAML: %w", err) to wrap the original error and preserve unwrapping support.
| return fmt.Errorf("failed to unmarshal YAML: %v", err) | |
| return fmt.Errorf("failed to unmarshal YAML: %w", err) |
Copilot uses AI. Check for mistakes.
| // First unmarshal into a generic map to preserve the case | ||
| var raw map[string]interface{} | ||
| if err = yaml.Unmarshal(data, &raw); err != nil { | ||
| return fmt.Errorf("failed to unmarshal YAML: %v", err) | ||
| } | ||
|
|
||
| // Convert to JSON bytes to use the struct's JSON tags | ||
| jsonBytes, err := json.Marshal(raw) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert to JSON: %v", err) | ||
| } | ||
|
|
||
| // Now unmarshal into the struct using JSON unmarshaler | ||
| // This will respect the json tags in the struct | ||
| if err = json.Unmarshal(jsonBytes, &application); err != nil { | ||
| return fmt.Errorf("failed to unmarshal JSON: %v", err) | ||
| } | ||
|
|
||
| // Verify the apiVersion was set correctly | ||
| if application.ApiVersion == "" { | ||
| // If still empty, try to get it directly from the raw map | ||
| if apiVer, ok := raw["apiVersion"].(string); ok { | ||
| application.ApiVersion = apiVer | ||
| } |
There was a problem hiding this comment.
[nitpick] Converting YAML to JSON through an intermediate map adds overhead; consider using a YAML library or decoder that respects JSON struct tags directly to avoid double marshalling.
| // First unmarshal into a generic map to preserve the case | |
| var raw map[string]interface{} | |
| if err = yaml.Unmarshal(data, &raw); err != nil { | |
| return fmt.Errorf("failed to unmarshal YAML: %v", err) | |
| } | |
| // Convert to JSON bytes to use the struct's JSON tags | |
| jsonBytes, err := json.Marshal(raw) | |
| if err != nil { | |
| return fmt.Errorf("failed to convert to JSON: %v", err) | |
| } | |
| // Now unmarshal into the struct using JSON unmarshaler | |
| // This will respect the json tags in the struct | |
| if err = json.Unmarshal(jsonBytes, &application); err != nil { | |
| return fmt.Errorf("failed to unmarshal JSON: %v", err) | |
| } | |
| // Verify the apiVersion was set correctly | |
| if application.ApiVersion == "" { | |
| // If still empty, try to get it directly from the raw map | |
| if apiVer, ok := raw["apiVersion"].(string); ok { | |
| application.ApiVersion = apiVer | |
| } | |
| // Unmarshal YAML directly into the struct | |
| if err = yaml.Unmarshal(data, &application); err != nil { | |
| return fmt.Errorf("failed to unmarshal YAML: %v", err) | |
| } | |
| // Verify the apiVersion was set correctly | |
| if application.ApiVersion == "" { | |
| return fmt.Errorf("apiVersion is missing in the YAML file") |
Copilot uses AI. Check for mistakes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
airlock-confluentinc
bot
force-pushed
the
cf-3631_fix_flink_application_payload
branch
2 times, most recently
from
b7aff45 to
67c21bf
Compare
This comment has been minimized.
This comment has been minimized.
airlock-confluentinc
bot
force-pushed
the
cf-3631_fix_flink_application_payload
branch
from
67c21bf to
ded01e8
Compare
Analysis Details8 Issues
Coverage and Duplications
Project ID: cli |
Release Notes
Bug Fixes
flink application create. It fails with YAML files, because it is unable to readapiVersion. For somereason, the YAML parser is looking forApiVersioninstead ofapiVersion, despite the json tag available. The fix for this at the moment is to not use the YAML library but instead first convert it to JSON and then unmarshal it.apiversion, but now it will be returned asapiVersionChecklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
Blast Radius
References
Test & Review
Has been manually tested for the example mentioned in the ticket
hello.yaml
And it passes