Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly report SQL insertion errors #15049

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Correctly report SQL insertion errors #15049

merged 2 commits into from
Jan 23, 2025

Conversation

miodvallat
Copy link
Contributor

Short description

When an sql insertion fails, not as a severe problem (such as a database connection timeout) but simply because no insertion took place, this should be reported as an error, but was not.

A consequence of this fix is that the test_GSSTSIG.py test now fail because pdnsutil import-key-zone now reports failure, as the sqlite database is empty at this state in the test run. The second commit addresses this by issueing pdnsutil create-zone first. (this operation targets the example.org zone which is not used/relevant in this test anyway)

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • added a random checkbox which noone will notice

@coveralls
Copy link

coveralls commented Jan 17, 2025

Pull Request Test Coverage Report for Build 12873120758

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 4 of 6 (66.67%) changed or added relevant lines in 2 files are covered.
  • 861 unchanged lines in 19 files lost coverage.
  • Overall coverage decreased (-0.4%) to 64.317%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/bindbackend/binddnssec.cc 1 3 33.33%
Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/dnsdistdist/dnsdist-backend.cc 1 66.19%
pdns/auth-secondarycommunicator.cc 1 63.84%
pdns/recursordist/aggressive_nsec.cc 2 66.39%
modules/lmdbbackend/lmdbbackend.cc 2 73.66%
pdns/iputils.cc 3 56.07%
modules/gpgsqlbackend/spgsql.cc 3 67.94%
pdns/recursordist/syncres.cc 4 80.25%
pdns/zoneparser-tng.cc 4 82.95%
pdns/recursordist/test-syncres_cc1.cc 5 89.76%
Totals Coverage Status
Change from base Build 12872106437: -0.4%
Covered Lines: 127051
Relevant Lines: 166445

💛 - Coveralls

@Habbie
Copy link
Member

Habbie commented Jan 20, 2025

, this should be reported as an error, but was not.

if I'm reading this (and comments on the previous PR) correctly, this does not report it as error, but as "unimplemented". I wonder if we should use some negative value? or make sure callers check for zero? note: I haven't been near any of this code recently

@miodvallat
Copy link
Contributor Author

I see what you mean. It is indeed better to return true (function implemented) but with a returned id of -1 in this case. I'll amend the PR shortly.

@Habbie Habbie merged commit d7910fc into PowerDNS:master Jan 23, 2025
81 checks passed
@miodvallat miodvallat deleted the zero branch January 23, 2025 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants