Skip to content

Fix: preserve trailing spaces for DATABASE in HAS_PERMS_BY_NAME#4128

Open
DervisAliDuman wants to merge 6 commits intobabelfish-for-postgresql:BABEL_5_X_DEVfrom
DervisAliDuman:fix/has_perms_by_name-database-trailing-space
Open

Fix: preserve trailing spaces for DATABASE in HAS_PERMS_BY_NAME#4128
DervisAliDuman wants to merge 6 commits intobabelfish-for-postgresql:BABEL_5_X_DEVfrom
DervisAliDuman:fix/has_perms_by_name-database-trailing-space

Conversation

@DervisAliDuman
Copy link

Description

What:
Previously, HAS_PERMS_BY_NAME would incorrectly strip trailing spaces from the @object_name parameter when the securable class was 'DATABASE'.
This caused permission checks to fail when the database name contained trailing spaces.

With this change:
Removed the rtrim() call on cs_as_securable for the 'DATABASE' securable class.
Now Babelfish preserves the exact string provided by the caller, including trailing spaces, aligning behavior with SQL Server.

Why:
SQL Server treats database names with and without trailing spaces as distinct.
Babelfish should match that behavior for accurate permission checks and T-SQL compatibility.

Verification:

CREATE DATABASE [ blank space ];
SELECT HAS_PERMS_BY_NAME(' blank space ', 'DATABASE', 'CREATE FUNCTION');

Result: Permission check now succeeds.


Issues Resolved

  • Fixes incorrect permission check behavior for databases with trailing spaces.
  • Aligns Babelfish behavior with SQL Server semantics for HAS_PERMS_BY_NAME.

Test Scenarios Covered

  • Use case based:
    Verified that HAS_PERMS_BY_NAME works with database names that include trailing spaces.

  • Boundary conditions:
    Tested database names with:

    • Single trailing space
    • Multiple trailing spaces
    • No trailing spaces (control case)
  • Negative test cases:
    Verified that permission checks still fail for users without CREATE FUNCTION privilege, regardless of trailing spaces.

  • Regression tests:
    Confirmed that other securable classes (OBJECT, SCHEMA, etc.) remain unaffected.

  • Minor/Major upgrade tests:
    Initialized Babelfish on a fresh database and verified behavior after restart.

  • Performance impact:
    No measurable impact. The change removes unnecessary rtrim() call for 'DATABASE' securable class only.


Check List

  • Commit signed off per DCO using --signoff
  • Change verified locally with sqlcmd on Babelfish-enabled PostgreSQL 17
  • Behavior matches SQL Server reference

Previously, HAS_PERMS_BY_NAME would incorrectly strip trailing spaces
from the @object_name parameter when the securable class was 'DATABASE'.
This caused permission checks to fail when the database name contained
trailing spaces.

This change removes the rtrim() on cs_as_securable for DATABASE
securable class, preserving the exact name provided by the caller.

Verified by:
  CREATE DATABASE [ blank space ];
  SELECT HAS_PERMS_BY_NAME(' blank space ', 'DATABASE', 'CREATE FUNCTION');

Result: Permission check now succeeds.

This aligns Babelfish behavior with SQL Server semantics, which treat
database names with and without trailing spaces as distinct.

Signed-off-by: Derviş Ali Duman <aduman387@gmail.com>
@DervisAliDuman DervisAliDuman force-pushed the fix/has_perms_by_name-database-trailing-space branch from f05370f to a0371b7 Compare October 1, 2025 12:46
…eflect trailing space handling

Signed-off-by: Derviş Ali Duman <aduman387@gmail.com>
@coveralls
Copy link
Collaborator

coveralls commented Oct 1, 2025

Pull Request Test Coverage Report for Build 18468011320

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 76.63%

Totals Coverage Status
Change from base Build 18455495732: 0.002%
Covered Lines: 51696
Relevant Lines: 67462

💛 - Coveralls

@DervisAliDuman DervisAliDuman force-pushed the fix/has_perms_by_name-database-trailing-space branch 2 times, most recently from 12f8235 to 317fefb Compare October 3, 2025 13:01
@DervisAliDuman DervisAliDuman force-pushed the fix/has_perms_by_name-database-trailing-space branch 2 times, most recently from ea34ea5 to f876527 Compare October 13, 2025 13:51
@DervisAliDuman
Copy link
Author

Hi @tanyagupta17, @forestkeeper , the tests are passing right now. Could you please review and approve them? Thank you in advance!

@DervisAliDuman
Copy link
Author

Hi @pranavJ23 , @rohit01010 , the tests are passing right now. Could you please review and approve them? Thank you in advance!

Copy link
Contributor

@pranavJ23 pranavJ23 left a comment

Choose a reason for hiding this comment

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

Hi @DervisAliDuman , thanks for working on this PR, I have a follow-up question:

CREATE DATABASE [  blank space    ];

Can you once confirm if T-SQL allows to make a database name ending with space/tab ?

@DervisAliDuman
Copy link
Author

Hi @DervisAliDuman , thanks for working on this PR, I have a follow-up question:

CREATE DATABASE [  blank space    ];

Can you once confirm if T-SQL allows to make a database name ending with space/tab ?

Yes @pranavJ23, you can run these test in your Test space:

DROP DATABASE IF EXISTS [  blank space    ];
CREATE DATABASE [  blank space    ];

SELECT HAS_PERMS_BY_NAME('[  blank space    ]','DATABASE','CREATE FUNCTION');
SELECT HAS_PERMS_BY_NAME('  blank space    ','DATABASE','CREATE FUNCTION');
SELECT HAS_PERMS_BY_NAME('blank space ','DATABASE','CREATE FUNCTION');

Also I would like to share the mssql results with you:
image

@tanscorpio7 tanscorpio7 added the In Review PR in review to be merged label Nov 1, 2025
@DervisAliDuman
Copy link
Author

Hello @pranavJ23, is this test case legit for you? You can open mssql and run these commands too:

DROP DATABASE IF EXISTS [  blank space    ];
CREATE DATABASE [  blank space    ];

SELECT HAS_PERMS_BY_NAME('[  blank space    ]','DATABASE','CREATE FUNCTION');
SELECT HAS_PERMS_BY_NAME('  blank space    ','DATABASE','CREATE FUNCTION');
SELECT HAS_PERMS_BY_NAME('blank space ','DATABASE','CREATE FUNCTION');

@DervisAliDuman
Copy link
Author

Hi @pranavJ23 and @rohit01010 , can you review it pls.


-- Assign each name accordingly
object_name = sys.babelfish_truncate_identifier(babelfish_remove_delimiter_pair(PG_CATALOG.rtrim(lower_object_name)));
object_name = sys.babelfish_truncate_identifier(babelfish_remove_delimiter_pair(lower_object_name));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this change produce a regression for case when originally database name do not have spaces but we passed spaces while calling HAS_PERMS_BY_NAME function in database name, eg:

create database [hello_db]
go

SELECT HAS_PERMS_BY_NAME('hello_db    ','DATABASE','CREATE FUNCTION');

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the old test and my tests combination support that test you wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that tests are there but is the behaviour correct ?
Can we confirm this behaviour with t-sql behaviour once ?

Copy link
Author

Choose a reason for hiding this comment

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

As I understand you want original mssql behaviour and I mentioned before with this test:

image

Isn't this confirmation that you wanted?

Copy link
Contributor

@pranavJ23 pranavJ23 Nov 26, 2025

Choose a reason for hiding this comment

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

Let me clarify it again :

  1. Make a db without any trailing spaces:
create database [hello_db]
go
  1. call function as
SELECT HAS_PERMS_BY_NAME('hello_db    ','DATABASE','CREATE FUNCTION');

Now we are calling the HAS_PERMS_BY_NAME with database as hello_db with trailing spaces , now this should return 1 as output as per t-sql behaviour, but since with your change you are removing the rtrim() so this could make the result 0 (instead of 1).

Note: in the tests you added contain the database name with spaces and we are comparing that, in the test I am pointing towards, database name does not have any spaces in it's name, but while calling HAS_PERMS_BY_NAME function we are adding spaces in database name

~~START~~
int
1
0
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain this change ?

Copy link
Author

Choose a reason for hiding this comment

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

Originally there were no trailing space on the right side while creating this database name and it was passing but it shouldn't beacuse it has wrong name withouth spaces. This test was passing beacuse it was rtrim()'ed the spaces. And there are no original databases that can test this bug. So I created My own tests.

@pranavJ23
Copy link
Contributor

Hi @DervisAliDuman , let me know if you have any opinion over: #4128 (comment)

@rohit01010 rohit01010 added the Pending Response Requested response from community reporter label Feb 23, 2026
@DervisAliDuman
Copy link
Author

DervisAliDuman commented Mar 4, 2026

Hi @DervisAliDuman , let me know if you have any opinion over: #4128 (comment)

Hi @pranavJ23 , I understand your concern. I just tested the exact scenario you mentioned in SSMS connected to a native SQL Server instance (SQL Server 2022). If we create [hello_db] without spaces, and then call SELECT HAS_PERMS_BY_NAME('hello_db ', 'DATABASE', 'CREATE FUNCTION');, T-SQL actually returns 0, not 1. It strictly matches the string for the DATABASE securable class and does not implicitly trim the input. Therefore, removing rtrim() from Babelfish correctly aligns our behavior with T-SQL without introducing a regression. I have attached the SSMS screenshot for this specific case before as well.

@rohit01010 rohit01010 removed the Pending Response Requested response from community reporter label Mar 6, 2026
@rohit01010
Copy link
Contributor

rohit01010 commented Mar 6, 2026

This is bit strange, when I tested above query I am also getting 1 in TSQL 2022.

1> SELECT cast(SERVERPROPERTY('ProductVersion') AS varchar(20)) AS ProductVersion, cast(SERVERPROPERTY('ProductLevel') AS varchar(20)) AS ProductLevel,cast(SERVERPROPERTY('Edition') as varchar(40)) AS Edition;
2> GO
ProductVersion       ProductLevel         Edition                                 
-------------------- -------------------- ----------------------------------------
16.0.4236.2          RTM                  Express Edition (64-bit)                

(1 rows affected)
1> CREATE DATABASE [hello_db]
2> go

1> SELECT HAS_PERMS_BY_NAME('hello_db    ','DATABASE','CREATE FUNCTION');
2> GO
           
-----------
          1

(1 rows affected)

@DervisAliDuman Can you please provide results of following queries in your TSQL Instance

SELECT @@VERSION
GO

SELECT cast(SERVERPROPERTY('ProductVersion') AS varchar(20)) AS ProductVersion, cast(SERVERPROPERTY('ProductLevel') AS varchar(20)) AS ProductLevel,cast(SERVERPROPERTY('Edition') as varchar(40)) AS Edition;
GO

CREATE DATABASE [hello_db]
GO

SELECT HAS_PERMS_BY_NAME('hello_db    ','DATABASE','CREATE FUNCTION');
GO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

In Review PR in review to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants