Skip to content

fix: handle DataValidationError raised by check_services_unique validator#500

Merged
Thanhphan1147 merged 3 commits intomainfrom
copilot/fix-data-validation-error
Apr 30, 2026
Merged

fix: handle DataValidationError raised by check_services_unique validator#500
Thanhphan1147 merged 3 commits intomainfrom
copilot/fix-data-validation-error

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 29, 2026 •
edited
Loading

What this PR does

Changes check_services_unique in HaproxyRouteRequirersData to add duplicate-service relation IDs to relation_ids_with_invalid_data instead of raising DataValidationError. This follows the existing pattern used by check_external_grpc_port_unique and the TCP library's check_ports_unique.

  • Library (lib/charms/haproxy/v2/haproxy_route.py): Replaced the raise with tracking duplicate service relation IDs, using the same defaultdict approach as the gRPC port validator.
  • Tests: Updated legacy test expectations and added a new test_check_services_unique covering mixed duplicate/unique services.

Why we need it

The _configure handler calls get_data(relations) which constructs HaproxyRouteRequirersData. The check_services_unique model validator raises DataValidationError on duplicate services, but this exception is unhandled — it propagates out of _configure and crashes the charm. The correct behavior is to mark offending relations as invalid and continue, which is what the other validators already do.

Test plan

All 251 unit tests pass. The new test validates that duplicate-service relations get their IDs added to relation_ids_with_invalid_data while unique services remain unaffected.

Review focus

The validator now silently discards all relations with duplicate service names (both/all duplicates, not just the second one). This matches the TCP check_ports_unique behavior.

Checklist

  • I followed the contributing guide
  • I added or updated the documentation (if applicable)
  • I updated docs/changelog.md with user-relevant changes
  • I added a change artifact for user-relevant changes in docs/release-notes/artifacts. If no change artifact is necessary, I tagged the PR with the label no-release-note.
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If integration test modules are used: I updated the workflow configuration
    (e.g., in .github/workflows/integration_tests.yaml, ensure the modules list is correct)
  • If this PR involves a Grafana dashboard: I added a screenshot of the dashboard
  • If this PR involves Terraform: terraform fmt passes and tflint reports no errors

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • gitlab.com
    • Triggering command: /usr/lib/git-core/git-remote-https /usr/lib/git-core/git-remote-https origin REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI linked an issue Apr 29, 2026 that may be closed by this pull request
DataValidationError raised by _get_data library method is not handled. #499
Closed
@Thanhphan1147
fix: handle DataValidationError for duplicate services in HaproxyRout…
d2ee10a 
…eRequirersData

Change check_services_unique validator to add duplicate-service relation
IDs to relation_ids_with_invalid_data instead of raising DataValidationError,
following the same pattern as check_external_grpc_port_unique and the TCP
library's check_ports_unique.

Agent-Logs-Url: https://github.com/canonical/haproxy-operator/sessions/fea73f9b-589e-4ff6-b73d-0768d1cbb90d

Co-authored-by: Thanhphan1147 <42444001+Thanhphan1147@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix DataValidationError handling in get_data method fix: handle DataValidationError raised by check_services_unique validator Apr 29, 2026
Copilot AI requested a review from Thanhphan1147 April 29, 2026 17:31
@Thanhphan1147 Thanhphan1147 marked this pull request as ready for review April 29, 2026 17:37
@Thanhphan1147 Thanhphan1147 requested a review from a team as a code owner April 29, 2026 17:37
swetha1654
swetha1654 reviewed Apr 30, 2026
View reviewed changes
Comment thread haproxy-operator/lib/charms/haproxy/v2/haproxy_route.py
Comment thread haproxy-operator/lib/charms/haproxy/v2/haproxy_route.py
@Thanhphan1147 Thanhphan1147 enabled auto-merge (squash) April 30, 2026 13:40
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

Test results for commit f43e3b7

Test coverage for f43e3b7

Name                                       Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------------
lib/charms/haproxy/v0/ddos_protection.py     154     51     34      8    64%   157-174, 183-187, 288, 316-318, 323, 326-330, 342-344, 381, 387, 393, 396, 424, 495-498, 510-529
src/charm.py                                  21      0      0      0   100%
src/state.py                                  43      0      0      0   100%
--------------------------------------------------------------------------------------
TOTAL                                        218     51     34      8    73%

Static code analysis report

Run started:2026-04-30 13:36:42.440913+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 333
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

Test results for commit f43e3b7

Test coverage for f43e3b7

Name                                         Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------------
lib/charms/haproxy/v0/ddos_protection.py       154     42     34      3    72%   157-174, 183-187, 265, 284, 415-418, 422-424, 459-478, 514-529
lib/charms/haproxy/v0/spoe_auth.py             158     55     32      2    59%   203, 304-306, 315, 354-381, 392-402, 441-442, 459-472, 484-501, 522-525, 529-531
lib/charms/haproxy/v1/haproxy_route_tcp.py     385    153     78      8    56%   209, 212, 281, 290-293, 297-300, 318-321, 336, 342-347, 447, 452, 829-832, 836, 863-874, 897-900, 904-906, 926-928, 1042-1083, 1087-1093, 1097, 1166-1195, 1266-1305, 1335-1337, 1362-1364, 1386-1390, 1409-1411, 1429-1431, 1438-1444, 1452-1454, 1462-1463, 1474-1481, 1494-1505, 1513-1534, 1546-1547, 1558-1559, 1570-1573, 1584-1585, 1614-1623, 1639-1642, 1658-1669, 1685-1688, 1706-1717, 1728-1729, 1737-1738, 1746-1747, 1758-1761
lib/charms/haproxy/v2/haproxy_route.py         386     53     98     26    82%   181, 257, 266-269, 294-297, 318-323, 673-674, 867->exit, 874, 900-911, 934-937, 941-943, 962-964, 1136-1142, 1146, 1343->1345, 1347->1349, 1349->1351, 1351->1353, 1353->1355, 1355->1358, 1393, 1401, 1406, 1409, 1434, 1462, 1466, 1470, 1493, 1513, 1522-1523, 1525->exit, 1561-1563, 1583, 1597, 1602-1604
src/charm.py                                   293     71     84     13    71%   102, 228, 236-252, 257, 262, 280, 291, 297-298, 332-352, 371, 471, 478-486, 514-527, 540-545, 554, 567-568, 575, 585, 595, 601-607, 623, 674-677, 683->682, 696-699
src/haproxy.py                                 125     31      6      2    75%   108-114, 134-156, 266-267, 270, 278-284, 312, 342-353, 365-367, 377-378
src/http_interface.py                           73     25      4      0    62%   74, 83, 92, 106-108, 126, 138, 150, 162, 170-175, 187, 194, 202, 217-227
src/state/charm_state.py                        78     15     14      4    79%   94-96, 101-102, 105, 150-155, 164, 216-218, 230-231
src/state/ddos_protection.py                    39      0      2      0   100%
src/state/exception.py                           1      0      0      0   100%
src/state/ha.py                                 30      1      2      1    94%   50
src/state/haproxy_route.py                     284     46     76      8    82%   161, 190-199, 256, 281, 332-360, 369, 378, 387, 396, 408, 446-472, 528, 568, 584-585, 602, 817-830
src/state/haproxy_route_tcp.py                 120     17     42      1    80%   92-94, 109->112, 147-160
src/state/ingress.py                            38      0      4      0   100%
src/state/ingress_per_unit.py                   32      0      4      0   100%
src/state/spoe_auth.py                          26      2      2      0    93%   63-64
src/state/tls.py                                39      7     12      4    78%   74, 77-78, 127-135, 141-142
src/state/validation.py                         46     23      8      1    44%   66-67, 71-98
src/tls_relation.py                             62      3     14      3    92%   87->86, 119-129, 141->143
----------------------------------------------------------------------------------------
TOTAL                                         2369    544    516     76    74%

Static code analysis report

Working... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
Run started:2026-04-30 13:36:57.107828+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 10650
  Total lines skipped (#nosec): 13
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 10

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

Test results for commit f43e3b7

Test coverage for f43e3b7

Name                      Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------
src/charm.py                 88     29     14      3    63%   97-98, 108-113, 138, 145-157, 161-176, 184-199
src/policy.py                66     37      2      0    43%   30-32, 37-38, 43-53, 58-59, 64-65, 78-92, 115-117, 140-141, 159-176, 187-197, 208-211
src/state/database.py        32      2      6      2    89%   76, 79
src/state/policy.py          62      2     10      2    94%   94, 106
src/state/validation.py      44     18      0      0    59%   57-59, 61-63, 74-87
---------------------------------------------------------------------
TOTAL                       292     88     32      7    68%

Static code analysis report

Run started:2026-04-30 13:36:44.633108+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1152
  Total lines skipped (#nosec): 10
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

Test results for commit f43e3b7

Test coverage for f43e3b7

Name                                        Stmts   Miss  Cover
---------------------------------------------------------------
haproxy_route_policy/__init__.py                0      0   100%
haproxy_route_policy/settings.py               30      1    97%
haproxy_route_policy/test_settings.py           4      0   100%
haproxy_route_policy/urls.py                    5      0   100%
manage.py                                      11      2    82%
policy/__init__.py                              0      0   100%
policy/admin.py                                22      5    77%
policy/apps.py                                  3      0   100%
policy/db_models.py                            50      2    96%
policy/management/__init__.py                   0      0   100%
policy/management/commands/__init__.py          0      0   100%
policy/middleware.py                           16      4    75%
policy/migrations/0001_initial.py               7      0   100%
policy/migrations/0002_rule.py                  5      0   100%
policy/migrations/0003_alter_rule_kind.py       4      0   100%
policy/migrations/__init__.py                   0      0   100%
policy/rule_engine.py                          42      6    86%
policy/serializers.py                          30      4    87%
policy/tests/__init__.py                        0      0   100%
policy/tests/test_auth.py                      36     20    44%
policy/tests/test_models.py                    85      0   100%
policy/tests/test_rule_engine.py              117      0   100%
policy/tests/test_views.py                    201      0   100%
policy/urls.py                                  3      0   100%
policy/views.py                                95     16    83%
---------------------------------------------------------------
TOTAL                                         766     60    92%

Static code analysis report

Run started:2026-04-30 13:36:40.785961+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1680
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

Test results for commit f43e3b7

Test coverage for f43e3b7

Name                               Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------
src/charm.py                          45      9      2      0    77%   65-91, 96-98
src/haproxy_spoe_auth_service.py      44     16      2      0    61%   56-64, 76-82, 93-117
src/state.py                          55     15      6      1    67%   64-66, 79, 125-146
------------------------------------------------------------------------------
TOTAL                                144     40     10      1    68%

Static code analysis report

Run started:2026-04-30 13:36:34.980870

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 409
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 1

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@Thanhphan1147 Thanhphan1147 merged commit 70f5726 into main Apr 30, 2026
108 checks passed
@Thanhphan1147 Thanhphan1147 deleted the copilot/fix-data-validation-error branch April 30, 2026 20:18
Thanhphan1147 added a commit that referenced this pull request May 4, 2026
@Thanhphan1147
add pr-best-practices skill and initial best-practices instructions
340d81b 
- .github/skills/pr-best-practices/SKILL.md: skill that analyzes merged
  LLM-authored PRs, compares first vs final commit, and synthesizes do/don't
  rules with concrete code examples into best-practices.instructions.md
- .github/skills/pr-best-practices/evals/evals.json: 3 eval cases covering
  substantive PR, validation-fix PR, and trivial (non-significant) PR
- .github/instructions/best-practices.instructions.md: initial rules extracted
  from PR #475 (relation contracts, reconcile flow, testing) and PR #500
  (validation error handling) with before/after code examples
- .gitignore: ignore *-workspace/ directories (skill eval scratch data)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Thanhphan1147 added a commit that referenced this pull request May 4, 2026
@Thanhphan1147
add pr-best-practices skill and initial best-practices instructions
a4d788b 
- .github/skills/pr-best-practices/SKILL.md: skill that analyzes merged
  LLM-authored PRs, compares first vs final commit, and synthesizes do/don't
  rules with concrete code examples into best-practices.instructions.md
- .github/skills/pr-best-practices/evals/evals.json: 3 eval cases covering
  substantive PR, validation-fix PR, and trivial (non-significant) PR
- .github/instructions/best-practices.instructions.md: initial rules extracted
  from PR #475 (relation contracts, reconcile flow, testing) and PR #500
  (validation error handling) with before/after code examples
- .gitignore: ignore *-workspace/ directories (skill eval scratch data)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seb4stien seb4stien seb4stien approved these changes

@f-atwi f-atwi f-atwi approved these changes

@Thanhphan1147 Thanhphan1147 Awaiting requested review from Thanhphan1147

@swetha1654 swetha1654 Awaiting requested review from swetha1654

Assignees

@Thanhphan1147 Thanhphan1147

Copilot code review Copilot

Labels

Libraries: OK

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DataValidationError raised by _get_data library method is not handled.

5 participants

@seb4stien @f-atwi @Thanhphan1147 @swetha1654