Replacing Perl report/dynamic_report endpoint to Go #8843
Conversation
…tfence into feature/go-dyn-reports
manage xyz-packetfence-xyz.services
…tfence into feature/go-dyn-reports
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 69 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
docs/api/spec/static/components/schemas/base.yaml:1
- The prevCursor and nextCursor types are changed to integer, but this conflicts with the new report API where cursors can be strings, arrays, or other types. This creates an inconsistency between the base schema and the actual pagination implementation used in reports.
Status:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| =head1 NAME | ||
| Tests for pf::condition::equals |
There was a problem hiding this comment.
The documentation comments incorrectly reference 'pf::condition::equals' when they should reference 'pf::condition::starts_with'. This creates confusion about what module is actually being tested.
|
|
||
| =head1 DESCRIPTION | ||
| Tests for pf::condition::equals |
There was a problem hiding this comment.
The documentation comments incorrectly reference 'pf::condition::equals' when they should reference 'pf::condition::starts_with'. This creates confusion about what module is actually being tested.
|
|
||
| use_ok("pf::condition::starts_with"); | ||
|
|
||
| my $filter = new_ok ( "pf::condition::starts_with", [value => 'test'],"Test pf::condition::equals constructor"); |
There was a problem hiding this comment.
The documentation comments incorrectly reference 'pf::condition::equals' when they should reference 'pf::condition::starts_with'. This creates confusion about what module is actually being tested.
| # if (defined($item->{type}) && $item->{type} eq 'sql' && exists $item->{cursor_type}) { | ||
| # if ($item->{cursor_type} eq 'multi_field') { | ||
| # push @expandable_params, qw(cursor_field cursor_default); | ||
| # } | ||
| # } |
There was a problem hiding this comment.
This commented-out code should be removed rather than left as a comment. The logic has been moved to line 31 where cursor_field and cursor_default are now always included in expandable_params. Dead code in comments reduces code clarity.
| # if (defined($item->{type}) && $item->{type} eq 'sql' && exists $item->{cursor_type}) { | |
| # if ($item->{cursor_type} eq 'multi_field') { | |
| # push @expandable_params, qw(cursor_field cursor_default); | |
| # } | |
| # } |
| submitter, err := NewKafkaSubmiter(&options) | ||
| if err != nil { | ||
| log.LogErrorf(ctx, "SetupKafka: failed to create Kafka submitter: %s", err.Error()) | ||
| return |
There was a problem hiding this comment.
When Kafka submitter creation fails, the function returns without setting aggregatorStarted to true, which means subsequent calls to SetupKafka will retry setup. However, the error is only logged. Consider whether this silent failure is appropriate or if the application should panic/exit, as Kafka functionality will be unavailable.
| return | |
| panic("SetupKafka: Kafka submitter creation failed, exiting") |
| score: | ||
| type: string | ||
| type: integer |
There was a problem hiding this comment.
The 'score' field type was changed from string to integer. This is a breaking API change that could affect existing API consumers who expect a string value. Ensure this change is intentional and properly communicated in release notes or API versioning.
| nullable: true | ||
| score: | ||
| type: string | ||
| type: integer |
There was a problem hiding this comment.
The 'score' field type was changed from string to integer. This is a breaking API change that could affect existing API consumers who expect a string value. Ensure this change is intentional and properly communicated in release notes or API versioning.
| type: integer | ||
| description: The cursor pointer to fetch paginated results (from `prevCursor` and `nextCursor`). |
There was a problem hiding this comment.
The cursor type was changed from string to integer. However, in the new report implementation (go/plugin/caddy2/api/reports.go), cursors can be strings, integers, arrays, or other types depending on the cursor_type (field, multi_field, offset). This type constraint is too restrictive and doesn't match the actual API behavior.
| type: integer | |
| description: The cursor pointer to fetch paginated results (from `prevCursor` and `nextCursor`). | |
| description: The cursor pointer to fetch paginated results (from `prevCursor` and `nextCursor`). | |
| oneOf: | |
| - type: string | |
| - type: integer | |
| - type: array | |
| - type: object |
| DATETIME => { type => 'string' }, | ||
| TIMESTAMP => { type => 'string' }, |
There was a problem hiding this comment.
Removed the 'format: date-time' and example from DATETIME field mapping. This makes the API spec less informative for API consumers who won't know the expected datetime format. Consider keeping the format specification for better API documentation.
| DATETIME => { type => 'string' }, | |
| TIMESTAMP => { type => 'string' }, | |
| DATETIME => { type => 'string', format => 'date-time' }, | |
| TIMESTAMP => { type => 'string', format => 'date-time' }, |
| # Check for duplicate schema names before merging | ||
| my %all_schemas; | ||
| for my $comp_set ($components, $components_deprecated, $components_static) { | ||
| if (exists $comp_set->{schemas}) { | ||
| for my $name (keys %{$comp_set->{schemas}}) { | ||
| if (exists $all_schemas{$name}) { | ||
| warn "WARNING: Duplicate schema '$name' found in multiple component sets\n"; | ||
| } | ||
| $all_schemas{$name} = 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate schema detection only warns but doesn't prevent the merge from proceeding. Depending on Perl's hash merge behavior, one schema will silently override the other. Consider making this a fatal error or providing a clear resolution strategy to prevent unexpected schema definitions.
Description
Port old report/dynamic_report API in Perl to Go while keeping compatibility.
Impacts
Todo
Enhancements