Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Zhdanov0 · 2023-10-16T20:54:59Z

Added the ability to change sql security opt without specifiying the entire body of the function/procedure/package.

ALTER FUNCTION [ {DETERMINISTIC | NOT DETERMINISTIC} ] [ SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY ]
Examples:
ALTER FUNCTION name DETERMINISTIC SQL SECURITY DEFINER
ALTER FUNCTION name SQL SECURITY INVOKER
ALTER FUNCTION name NOT DETERMINISTIC DROP SQL SECURITY
ALTER PROCEDURE SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY
ALTER PACKAGE SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY

Zhdanov0 · 2023-10-16T22:10:18Z

Added the ability to change sql security (definer / invoker / drop) to the existing syntax ALTER FUNCTION { DETERMINISTIC | NOT DETERMINISTIC }.
So that you can write ALTER FUNCTION funcname DETERMINISTIC SQL SECURITY DEFINER

I did it by analogy with "alter_domain_ops"

The deterministic field type was changed from bool to TriState.
The third "null" state appears if you write: ALTER FUNCTION funcname SQL SECURITY DEFINER. Then, with a partial update, deterministic_flag is not set to false.
If you do not change it to TriState, then ALTER FUNCTION funcname SQL SECURITY DEFINER
will set deterministic_flag to false, because in createAlterFunctionNode it defaults to false.

The sql security field type was changed from TriState to std::optional because the SS_DROP state was added.
Added sql security clauses:
states for full update: definer / invoker / nothing
for partial update: definer / invoker / drop
In full update prohibited "drop sql security"
in partial update prohibited "nothing" because it will be infinite recursion.

sim1984 · 2023-10-17T06:23:51Z

SQL SECURITY must be able to change in procedures too. Moreover, it is desirable to have a similar syntax.

sim1984 · 2023-10-18T12:47:24Z

See also #7744

dyemanov · 2023-10-24T09:08:55Z

I'm fine with this implementation, please go ahead for stored procedures.

asfernandes · 2023-10-26T00:32:52Z

src/dsql/PackageNodes.epp

@@ -215,6 +215,9 @@ string CreateAlterPackageNode::internalPrint(NodePrinter& printer) const

 DdlNode* CreateAlterPackageNode::dsqlPass(DsqlCompilerScratch* dsqlScratch)
 {
+	if(alter && !items)


Suggested change

if(alter && !items)

if (alter && !items)

asfernandes · 2023-10-26T00:33:10Z

src/dsql/PackageNodes.epp

 	if (alter)
 	{
-		if (!executeAlter(tdbb, dsqlScratch, transaction))
+		if(alterIndividualParameters)


Suggested change

if(alterIndividualParameters)

if (alterIndividualParameters)

asfernandes · 2023-10-26T00:33:21Z

src/dsql/PackageNodes.epp

-		if (!executeAlter(tdbb, dsqlScratch, transaction))
+		if(alterIndividualParameters)
+		{
+			if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))


Suggested change

if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))

if (!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))

asfernandes · 2023-10-26T00:34:01Z

src/dsql/PackageNodes.epp

+		MODIFY PKG
+			if (ssDefiner.has_value())
+			{
+				if(ssDefiner.value() != SqlSecurity::SS_DROP)


Suggested change

if(ssDefiner.value() != SqlSecurity::SS_DROP)

if (ssDefiner.value() != SqlSecurity::SS_DROP)

asfernandes · 2023-10-26T00:35:06Z

src/dsql/parse.y

@@ -2707,9 +2707,13 @@ procedure_clause
 	| external_procedure_clause
 	;

+%type <createAlterProcedureNode> change_opt_procedure_clause
+change_opt_procedure_clause


What is this empty rule?

It's not empty, the definition is below.

And why you "declared it" here?

%type <createAlterProcedureNode> change_opt_procedure_clause change_opt_procedure_clause ;

asfernandes · 2023-10-26T00:39:12Z

src/dsql/parse.y

@@ -2966,6 +2987,16 @@ package_clause
 		}
 	;

+%type <createAlterPackageNode> change_opt_package_clause
+change_opt_package_clause


Maybe change_opt_package_clause and change_opt_procedure_clause could be put directly in the alter rules?
I think these rules has not so great names to be put standalone and they are not reused.

I made rules change_opt_package_clause and change_opt_procedure_clause to separate full alter and partial alter. I can change the names to partial_alter_package_clause and partial_alter_procedure_clause. This way it will be clearer and will separate full alter and partial alter.

You call it partial and I call it just another form of alter. Every alter is partial, they change what was specified. So my suggestion to just use another branch of the original rules directly.

"Every alter is partial, they change what was specified." - is not correct for functions, procedures and packages.
Functions/procedures/packages change all parameters, even those that were not specified. For example:

ALTER FUNCTION func1 (str varchar(5)) RETURNS INT AS DECLARE result INTEGER; BEGIN result = POSITION('a', str); RETURN result; END;

The deterministic and sql security parameters were not specified, but are still being modified
(reset).
I will call this a full alter (the executeAlter method is used).

Now partial alter (executeAlterIndividualParameters method is used):
Here parameters that are not specified will not be modified. Only the individual specified parameters will be changed.
For example:

ALTER FUNCTION name SQL SECURITY INVOKER

The deterministic parameter is not specified here and it is not modified.

My opinion:
This is different behavior and it should be in different rules.
But it is better to change the name to a more clarifying one: partial_alter_package_clause for example.

So we have a big inconsistency with triggers:

SQL> create table t1 (n1 integer)! SQL> create trigger t1_ins inactive before insert position 4 on t1 SQL SECURITY DEFINER as begin end! SQL> show trigger t1_ins! Triggers on Table T1: T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER Trigger text: ============================================================================= as begin end ============================================================================= SQL> alter trigger t1_ins as begin end! SQL> show trigger t1_ins! Triggers on Table T1: T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER Trigger text: ============================================================================= as begin end =============================================================================

asfernandes · 2023-10-26T00:40:05Z

src/dsql/DdlNodes.epp

+		{
 			altered = true;
+		}


Suggested change

{

altered = true;

}

altered = true;

dyemanov · 2023-11-08T18:55:12Z

Unless there will be other issues posted, I'll merge this PR later this week.

dyemanov · 2023-11-10T07:38:12Z

@Zhdanov0, please resolve the existing conflicts.

pavel-zotov · 2023-11-12T10:18:54Z

QA is covered by test for #7744

Add ability to change sql security option in alter function

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

9b823e6

Alexander Zhdanov added 2 commits October 25, 2023 19:27

added the ability to change sql security opt in alter procedure

5b62093

added the ability to change sql security option in alter package

695082e

Zhdanov0 changed the title ~~Added the ability to change sql security option in ALTER FUNCTION~~ Added the ability to change sql security option without specifying the entire body of the function/procedure/package Oct 25, 2023

asfernandes reviewed Oct 26, 2023

View reviewed changes

refactoring rules for partial alters

b774645

Zhdanov0 and others added 2 commits November 10, 2023 13:12

Merge branch 'master' into master

93c1b1f

triState namespace fix

ecc0fec

dyemanov merged commit 26a78fa into FirebirdSQL:master Nov 10, 2023

dyemanov assigned Zhdanov0 Nov 10, 2023

dyemanov added component: engine type: improvement affect-version: 6.0 Initial fix-version: 6.0 Alpha 1 and removed affect-version: 6.0 Initial labels Nov 10, 2023

pavel-zotov added the qa: covered by another tests label Nov 12, 2023

dyemanov added a commit that referenced this pull request Nov 13, 2023

Postfix for PR #7807

9ed5a4e

dyemanov linked an issue Nov 24, 2023 that may be closed by this pull request

Provide ability to run "ALTER SQL SECURITY DEFINER / INVOKER" without specifying further part of proc/func/package #7744

Closed

dyemanov added the rdb label Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Zhdanov0 commented Oct 16, 2023 •

edited

Loading

Zhdanov0 commented Oct 16, 2023

sim1984 commented Oct 17, 2023

sim1984 commented Oct 18, 2023

dyemanov commented Oct 24, 2023

asfernandes Oct 26, 2023

asfernandes Oct 26, 2023

asfernandes Oct 26, 2023

asfernandes Oct 26, 2023

asfernandes Oct 26, 2023

Zhdanov0 Oct 26, 2023

asfernandes Oct 26, 2023

asfernandes Oct 26, 2023

Zhdanov0 Oct 26, 2023

asfernandes Oct 26, 2023

Zhdanov0 Oct 26, 2023 •

edited

Loading

asfernandes Oct 27, 2023

asfernandes Oct 26, 2023

dyemanov commented Nov 8, 2023

dyemanov commented Nov 10, 2023

pavel-zotov commented Nov 12, 2023

	if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))
	if (!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))

	if(ssDefiner.value() != SqlSecurity::SS_DROP)
	if (ssDefiner.value() != SqlSecurity::SS_DROP)

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Conversation

Zhdanov0 commented Oct 16, 2023 • edited Loading

Zhdanov0 commented Oct 16, 2023

sim1984 commented Oct 17, 2023

sim1984 commented Oct 18, 2023

dyemanov commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhdanov0 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyemanov commented Nov 8, 2023

dyemanov commented Nov 10, 2023

pavel-zotov commented Nov 12, 2023

Zhdanov0 commented Oct 16, 2023 •

edited

Loading

Zhdanov0 Oct 26, 2023 •

edited

Loading