Skip to content
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

fix: DatabaseMetaData.getFunctions should not limit the search to the search_path if the schema is provided #1633

Merged
merged 2 commits into from Dec 5, 2019

Conversation

davecramer
Copy link
Member

@davecramer davecramer commented Dec 3, 2019

Currently getFunctions adds pg_function_is_visible to the sql looking for functions in all cases. If the user provides a schema but it is not on the search path no functions will be found. This seems counter-intuitive.

With this PR if the user provides a schema then that schema will be searched. If the schema is empty or null then the search path will be searched for the function.

@davecramer
Copy link
Member Author

@davecramer davecramer commented Dec 3, 2019

@sehrope this is an area where you would have quite a bit of feedback I would think?

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 3, 2019

Codecov Report

Merging #1633 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1633      +/-   ##
============================================
+ Coverage     69.02%   69.06%   +0.03%     
- Complexity     4068     4073       +5     
============================================
  Files           181      181              
  Lines         16861    16863       +2     
  Branches       2760     2760              
============================================
+ Hits          11638    11646       +8     
+ Misses         3947     3945       -2     
+ Partials       1276     1272       -4

@davecramer davecramer merged commit 8106d3d into pgjdbc:master Dec 5, 2019
3 checks passed
@davecramer davecramer deleted the fixgetfunctions branch Dec 5, 2019
@davecramer
Copy link
Member Author

@davecramer davecramer commented Jun 4, 2021

@sehrope, @vlsi so ... squirrel is having an issue with this change in that we now limit NULL schema searches to the search_path. The spec says nothing about this. I'm inclined to add a switch.

@sehrope
Copy link
Member

@sehrope sehrope commented Jun 4, 2021

@dave Sorry but I think my GH email notification filters were messed up in 2019 as I never saw this one.

We were already limiting everything to what's visible prior to this, no?

My read of the new behavior is:

  • If you specify a schema, list everything in the schema.
  • If you do not specify a schema, list everything that's visible.

So this should be an expansion of what's visible when a schema is explicitly specified (e.g. show me everything in foo). That sounds reasonable to me so what do they want changed?

@davecramer
Copy link
Member Author

@davecramer davecramer commented Jun 4, 2021

Well they want to see everything in the database if it is NULL. The spec could be construed that way. What do other db's do ? Do they have a notion of search_path ?

@sehrope
Copy link
Member

@sehrope sehrope commented Jun 4, 2021

Oh they want it expanded further. Yes that makes sense to me. There's no reason to limit to the search path when listing objects in metadata functions. Otherwise the only way to list everything would be to list all the schemas and then make N requests (one per schema).

The MariaDB JDBC driver doesn't filter by visibility: https://github.com/mariadb-corporation/mariadb-connector-j/blob/master/src/main/java/org/mariadb/jdbc/DatabaseMetaData.java#L3724-L3737

The MSSQL driver calls SP_STORED_PROCEDURES(...) which considers a NULL for the schema to not filter by schema: https://github.com/microsoft/mssql-jdbc/blob/0f3e0a10f2f85b5b357811907fdb487d63e49830/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java#L818-L840

@davecramer
Copy link
Member Author

@davecramer davecramer commented Jun 4, 2021

Ok, So this is going to introduce a behavioral change. Thoughts on how to deal with this ?

@sehrope
Copy link
Member

@sehrope sehrope commented Jun 4, 2021

How about having it respect HIDE_UNPRIVILEGED_OBJECTS? That's a connection property, defaulting to false, and it's how getTables(...) and a bunch of other methods make that determination:

if (connection.getHideUnprivilegedObjects()) {
select += " AND has_table_privilege(c.oid, "
+ " 'SELECT, INSERT, UPDATE, DELETE, RULE, REFERENCES, TRIGGER')";
}

So the change would be showing it by default, but I'd argue that's a bug fix. It really should be showing everything just like the rest of the metadata functions.

@davecramer
Copy link
Member Author

@davecramer davecramer commented Jun 4, 2021

Might as well just fix it.

@gerdwagner
Copy link

@gerdwagner gerdwagner commented Jun 5, 2021

As we were discussing this issue here
https://sourceforge.net/p/squirrel-sql/bugs/1416/

I repeat my argument to support Dave's suggestion:
What made me believe that procedures/functions from all schemas should be returned is the passage
"null means that the schema name should not be used to narrow the search"
which can be found in the schema parameter's description of getProcedures(...) and getFunctions(...) in java.sql.DataBaseMetaData.

davecramer added a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
… search_path if the schema is provided (pgjdbc#1633)

* fix: DatabaseMetaData.getFunctions should not limit the search to the search_path if the schema is provided
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.

None yet

4 participants