GH-133: [GO] Tabular and Graph Attribute Validator #233
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements tabular and graph attribute validators as requested in issue GH-133. The implementation adds validation functions to ensure that tabular data contains only 'columns' and 'rows' fields, and graph data contains only 'nodes' and 'edges' fields.
- Added
validateTabularAttributesfunction to validate tabular data structure - Added
validateGraphAttributesfunction to validate graph data structure - Comprehensive test coverage for both validators including edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| design/crud-api/pkg/schema/utils.go | Implements the core validation functions for tabular and graph attributes |
| design/crud-api/pkg/schema/utils_test.go | Adds comprehensive test cases covering valid inputs, missing fields, extra fields, and edge cases |
| required := []string{"columns", "rows"} | ||
| // Check required keys | ||
| for _, key := range required { |
There was a problem hiding this comment.
[nitpick] Consider extracting the required fields as package-level constants to avoid duplication and improve maintainability. This would make it easier to modify the required fields in the future.
| required := []string{"columns", "rows"} | |
| // Check required keys | |
| for _, key := range required { | |
| // Check required keys | |
| for _, key := range TabularRequiredFields { |
|
|
||
| // validateGraphAttributes checks that only 'nodes' and 'edges' are present as keys in the map. | ||
| func validateGraphAttributes(obj map[string]interface{}) error { | ||
| required := []string{"nodes", "edges"} |
There was a problem hiding this comment.
[nitpick] Consider extracting the required fields as package-level constants to avoid duplication and improve maintainability. This would make it easier to modify the required fields in the future.
| if key != "columns" && key != "rows" { | ||
| return fmt.Errorf("unexpected key '%s' in tabular data; only 'columns' and 'rows' are allowed", key) |
There was a problem hiding this comment.
The hardcoded field names are duplicated from the required slice. Consider using a loop or map lookup against the required fields to eliminate this duplication.
| if key != "columns" && key != "rows" { | |
| return fmt.Errorf("unexpected key '%s' in tabular data; only 'columns' and 'rows' are allowed", key) | |
| if !contains(required, key) { | |
| return fmt.Errorf("unexpected key '%s' in tabular data; only %v are allowed", key, required) |
| required := []string{"nodes", "edges"} | ||
| // Check required keys | ||
| for _, key := range required { | ||
| if _, exists := obj[key]; !exists { | ||
| return fmt.Errorf("graph data must contain '%s' field", key) | ||
| } | ||
| } | ||
| // Check for extra keys | ||
| for key := range obj { | ||
| if key != "nodes" && key != "edges" { |
There was a problem hiding this comment.
The hardcoded field names are duplicated from the required slice. Consider using a loop or map lookup against the required fields to eliminate this duplication.
| required := []string{"nodes", "edges"} | |
| // Check required keys | |
| for _, key := range required { | |
| if _, exists := obj[key]; !exists { | |
| return fmt.Errorf("graph data must contain '%s' field", key) | |
| } | |
| } | |
| // Check for extra keys | |
| for key := range obj { | |
| if key != "nodes" && key != "edges" { | |
| required := map[string]struct{}{ | |
| "nodes": {}, | |
| "edges": {}, | |
| } | |
| // Check required keys | |
| for key := range required { | |
| if _, exists := obj[key]; !exists { | |
| return fmt.Errorf("graph data must contain '%s' field", key) | |
| } | |
| } | |
| // Check for extra keys | |
| for key := range obj { | |
| if _, allowed := required[key]; !allowed { |
|
@sehansi-9 we merged in a refactor yesterday. Could you please rebase? |
sehansi-9
force-pushed
the
feature/protobuf_validator
branch
from
676e6a5 to
fa3dc1d
Compare
|
rebased |
2 participants
issue: #133
implemented: https://github.com/LDFLK/launch/issues/134#issuecomment-3060945413