Re: [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table

[Nikita Pettik <korablev@tarantool.org>]

On 18 Dec 14:00, Chris Sosnin wrote:
> Dropping table with sql removes everything
> associated with it but grants, which is
> inconsistent. Generating code for it fixes this bug.

Feel free to use up to 77 chars in commit message. You can enable
auto-formatting in vim:

au FileType gitcommit setlocal tw=72

Or you can manually highlight commit message and format with :gq
 
> Closes #4546
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
> issue: https://github.com/tarantool/tarantool/issues/4546

You don't have to put these links in each patch, they are required
only in cover letter.
 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce63..f1433645a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> +static void
> +vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
> +			uint32_t object_id, struct access *access)
> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	assert(v != NULL);
> +	/*
> +	 * Get uid of users through access array
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);
> +	bool had_grants = false;

As a rule we use present time: has_grants.

> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (!access[token].granted)
> +			continue;
> +		had_grants = true;

Personally I wouldn't bother with separate variable solely to
display comment. Let's keep it tho.

> +		const struct user *user = user_find_by_token(token);
> +		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
> +		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +			      object_type, P4_STATIC);
> +		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
> +		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
> +		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
> +	}
> +	if (had_grants)
> +		VdbeComment((v, "Remove %s grants", object_type));
> +	sqlReleaseTempRange(parser, key_reg, 4);
> +}
> +
>  /**
>   * Generate code to drop a table.
>   * This routine includes dropping triggers, sequences,
> @@ -1538,6 +1578,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  {
>  	struct Vdbe *v = sqlGetVdbe(parse_context);
>  	assert(v != NULL);
> +	/*
> +	 * Remove all grants associated with
> +	 * with the table being dropped.

Nit: double with.

The rest is OK. LGTM and pushed to master.

> +	 */
> +	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
> +				space->access);
>  	/*