Skip to content

PG-1537 Error on required arguments #267

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

Conversation

AndersAstrand
Copy link
Collaborator

Do not accept NULL or the empty string for required arguments in our user facing functions.

I'm a bit unsure of where the helper required_text_argument() belongs, any input is appreciated.

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.50%. Comparing base (1b2512a) to head (7261edf).

❌ Your project status has failed because the head coverage (79.50%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           TDE_REL_17_STABLE     #267      +/-   ##
=====================================================
+ Coverage              79.38%   79.50%   +0.11%     
=====================================================
  Files                     22       22              
  Lines                   2532     2537       +5     
  Branches                 396      400       +4     
=====================================================
+ Hits                    2010     2017       +7     
+ Misses                   444      443       -1     
+ Partials                  78       77       -1     
Components Coverage Δ
access 83.06% <ø> (ø)
catalog 86.69% <100.00%> (+0.30%) ⬆️
common 92.50% <ø> (ø)
encryption 71.90% <ø> (ø)
keyring 72.07% <ø> (ø)
src 59.56% <ø> (ø)
smgr 98.01% <ø> (ø)
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AndersAstrand AndersAstrand force-pushed the tde/validate-input-in-user-functions branch from 3f5fb21 to ddd2519 Compare April 25, 2025 17:54

if (!PG_ARGISNULL(arg_num))
{
value = text_to_cstring(PG_GETARG_TEXT_PP(arg_num));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have a separate error message for this, e.g. like the below?

# SELECT * FROM pg_create_physical_replication_slot('');
ERROR:  replication slot name "" is too short

Copy link
Collaborator Author

@AndersAstrand AndersAstrand Apr 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see the value in that. But we can if we want to! :)

If we do that we should probably only do it when creating stuff and accept the empty string (and never find anything) when looking things up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I do prefer that since it is after all a plugin which will be used by anyone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version of the branch pushed, please take a look. I think this is better.

@AndersAstrand AndersAstrand force-pushed the tde/validate-input-in-user-functions branch from ddd2519 to 8539c49 Compare April 29, 2025 11:25
Copy link
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Passing NULL here crashed the server, also do not accept the empty
string as a key name.
Previously these simply crashed the server on NULL input.
Also update the error message for too long provider names to make it
symmetrical.
@AndersAstrand AndersAstrand force-pushed the tde/validate-input-in-user-functions branch from 8539c49 to 7261edf Compare April 29, 2025 14:15
@AndersAstrand AndersAstrand merged commit 52d957e into percona:TDE_REL_17_STABLE Apr 29, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants