Fix issue with PostgreSQL: org.postgresql.util.PSQLException #6050

nenaraab · 2018-11-06T10:44:35Z

When trying to use Spring Security ACL, I am facing the error: org.postgresql.util.PSQLException: ERROR: operator does not exist: bigint = character varying.

As recommended in the Spring.io documentation i've setup the acl tables in my PostgreSQL database, with column acl_object_identity.object_id_identity of type varchar(36).

IMHO the object_id_identity in the jdbcTemplate method calls MUST always be passed as a character sequence.

This relates to issue:
#5508

pivotal-issuemaster · 2018-11-06T10:44:36Z

@nenaraab Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2018-11-06T10:45:23Z

@nenaraab Thank you for signing the Contributor License Agreement!

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

rwinch · 2018-11-28T22:40:13Z

Thank you for the PR @nenaraab!

Could you please rebase off master?
Could you please use String.valueOf as suggested by @mangei?

If you need any help with the rebase, please let me know and I'd be happy to help.

nenaraab · 2018-11-29T09:35:15Z

@rwinch
Thanks for cooperation! I've updated the PR. This will fix all my current PostgreSQL issues...

rwinch

Thanks for the fast turnaround @nenaraab! Can you add tests for potential null value and to ensure it is converted to a String for non-string types?

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

nenaraab · 2018-11-29T22:01:07Z

@rwinch
no problem! Please confirm... i will add some checks (using Assert) at the right places to make it bulletproof

check that identifier is not NULL (if not already checked in the calling public method)
check that Spring.valueOf does not return null

rwinch · 2018-11-29T22:58:59Z

Thanks for the fast reply @nenaraab!

Sorry for the confusion. I was not very clear. What I meant was to do something like this:

String id = parentIdentity.getIdentifier() == null ? 
    null : parentIdentity.getIdentifier().toString()

As for the testing I meant to add some tests that break before your changes and are fixed after the changes. Also add a test that validates that a null value is passed through if getIdentifier() is passed in. I'd probably add them to JdbcAclServiceTests.

To make testing more realistic, I'd:

Change JdbcTemplate to be JdbcOperations
Add a constructor that allows injecting the JdbcOperations and LookupStrategy

Then the tests could inject a Mock JdbcOperations for the tests mentioned above.

Thanks again for your help with this ticket! If you have any questions, don't hesitate to reach out.

nenaraab · 2018-11-30T17:22:49Z

@rwinch
It's not that i dislike to implement a unittest... and thanks for the guidance here.

But as ObjectIdentity.getIdentifier() never returns null (see documentation, see all constructor implementations) i don't get the point of introducing a null check at all.

Furthermore I've checked the ObjectIdentityImplTests thoroughly. All not null assertions according to the identifier are covered by tests.

acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java

rwinch

Thanks! You are right about not needing the null checks. I have provided feedback inline.

nenaraab · 2018-12-11T12:40:27Z

@rwinch
great, added two test cases to test the code change.

Furthermore I've added integration tests (using hsql in-memory database) to test also a proper conversion of the fetched entries, and the find-children sql queries as well.

rwinch

Thanks for all the tests @nenaraab! I have another round of feedback inline.

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

rwinch

Thanks for the fast turnaround! I have provided feedback inline.

rwinch · 2018-12-14T19:39:09Z

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

@@ -77,9 +78,14 @@
 	// ===================================================================================================

 	public JdbcAclService(DataSource dataSource, LookupStrategy lookupStrategy) {
+		this(new JdbcTemplate(dataSource), lookupStrategy);
 		Assert.notNull(dataSource, "DataSource required");


this assertion is not necessary because JdbcTemplate validates that the dataSource is not null.

I'm so sorry!
this @Nullable was missleading...
but you're right: the method afterPropertiesSet raises an exception, so I've removed the assertion. Sorry for inconvenience...

No problem. It is a bit confusing and due the fact there is also a default constructor.

Thanks for the fast turnaround!

When setup the acl tables as specified in the Spring.io documentation I have faced the following error on a PostgreSql database: org.postgresql.util.PSQLException: ERROR: operator does not exist: bigint = character varying. This is because the acl_object_identity.object_id_identity column is of type varchar(36) but it is not necessarily accessed with a value of type String. - JdbcAclService / JdbcMutableAclService: SQL query must match object_id_identity column specification - JdbcAclService: changed JdbcTemplate to JdbcOperations for testability - JdbcAclServiceTest: Increased test coverage, the integration tests using embedded db relates to this commit thomasdarimont@cd8d207 Fixes gh-5508

juzerali · 2021-06-11T06:26:15Z

In which version is this fix available?

jzheaux · 2021-06-17T18:09:43Z

You can see by looking at the commit hash - d1a754f. In the comment at the top, the versions are listed. Let me know if I can help further.

juzerali · 2021-06-18T03:57:43Z

Got it, so it is available since v5.2. Thanks @jzheaux

mangei reviewed Nov 25, 2018

View reviewed changes

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java Outdated Show resolved Hide resolved

rwinch added the status: waiting-for-feedback We need additional information before we can continue label Nov 28, 2018

rwinch mentioned this pull request Nov 28, 2018

Spring Security ACL: No operator matches the given name and argument type #5508

Closed

rwinch requested changes Nov 29, 2018

View reviewed changes

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java Show resolved Hide resolved

rwinch reviewed Nov 30, 2018

View reviewed changes

acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java Outdated Show resolved Hide resolved

rwinch requested changes Nov 30, 2018

View reviewed changes

rwinch requested changes Dec 12, 2018

View reviewed changes

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java Outdated Show resolved Hide resolved

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java Outdated Show resolved Hide resolved

rwinch requested changes Dec 14, 2018

View reviewed changes

rwinch removed the status: waiting-for-feedback We need additional information before we can continue label Dec 17, 2018

rwinch self-assigned this Dec 17, 2018

rwinch approved these changes Dec 17, 2018

View reviewed changes

rwinch merged commit d1a754f into spring-projects:master Dec 17, 2018

rwinch added status: duplicate A duplicate of another issue in: core An issue in spring-security-core type: bug A general bug labels Dec 17, 2018

rwinch added this to the 5.2.0.M1 milestone Dec 17, 2018

Fix issue with PostgreSQL: org.postgresql.util.PSQLException #6050

Fix issue with PostgreSQL: org.postgresql.util.PSQLException #6050

Uh oh!

Conversation

nenaraab commented Nov 6, 2018

Uh oh!

pivotal-issuemaster commented Nov 6, 2018

Uh oh!

pivotal-issuemaster commented Nov 6, 2018

Uh oh!

Uh oh!

rwinch commented Nov 28, 2018

Uh oh!

nenaraab commented Nov 29, 2018

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nenaraab commented Nov 29, 2018

Uh oh!

rwinch commented Nov 29, 2018

Uh oh!

nenaraab commented Nov 30, 2018

Uh oh!

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

nenaraab commented Dec 11, 2018

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

rwinch Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

nenaraab Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

juzerali commented Jun 11, 2021

Uh oh!

jzheaux commented Jun 17, 2021

Uh oh!

juzerali commented Jun 18, 2021

Uh oh!

Uh oh!

nenaraab Dec 14, 2018 •

edited

Loading