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

Add additional tests for getTablePrivileges() for materialized views and foreign tables #2060

Closed
sehrope opened this issue Feb 14, 2021 · 4 comments · Fixed by #2209
Closed
Labels
good first issue Good for newcomers

Comments

@sehrope
Copy link
Member

sehrope commented Feb 14, 2021

Placeholder issue for adding additional test cases for materialized views and foreign tables. See #2049 for details.

This should be straight forward to add would be great first time contribution as it's totally isolated in new test code.

@jorsol jorsol added the good first issue Good for newcomers label Feb 15, 2021
@mgrobaker
Copy link
Contributor

mgrobaker commented Jul 2, 2021

Could you elaborate on this a little? I am working on this as my first issue. I've gotten everything up and running, and all tests are passing. I'm now in this test class and trying to work on the issue itself.

Two questions off the top of my head - but feel free to add extra color as well

  1. Are there existing tests already in here that I can use as a basis for making new methods to test the views and foreign tables? Or is that part of the issue, coming up with such tests.
    (It is a little hard for me to read through the whole class DatabaseMetaDataTest, given that there are several database objects being tested and not a ton of comments. But I can do that. I just wanted to check that was necessary before going through it all.)

  2. Also, the title of the issue speaks to getTablePrivileges(). I don't exactly understand why. I see getViewPrivileges() was added. Is the idea to beef up the tests in just this specific method, or to make new methods to test just privileges on views and foreign tables?

@sehrope
Copy link
Member Author

sehrope commented Jul 2, 2021

That PR fixed a bug where the getTablePrivileges(...) metadata method was only returning results for tables and partitioned tables, but not views, materialized views, and foreign tables. The fix included all of those relation types but the additional test only covered views with the new testViewPrivileges() method.

The PR I'm suggesting someone create would be two parts:

  1. Add testMaterializedViewPrivileges() which would create a materalized view, verify that it appears in getTablePrivileges(), and then drop the materialized view in the teardown.
  2. Add testForeignTablePrivileges() which would do the same with a foreign table.

That second one is a bit more complicated as a foreign table quite a bit more plumbing to get set up (an fdw, a foreign server, etc). So I'd suggest just doing just the materialized view piece.

@mgrobaker
Copy link
Contributor

Got it, thanks - this is clear now

mgrobaker added a commit to mgrobaker/pgjdbc that referenced this issue Jul 6, 2021
Add to TestUtil and also to DatabaseMetaData setup and teardown

fixes pgjdbc#2060
mgrobaker added a commit to mgrobaker/pgjdbc that referenced this issue Jul 6, 2021
make "matviewtest" all lowercase so it can be found in pg catalog, which lowercases all names

fixes pgjdbc#2060
@mgrobaker
Copy link
Contributor

Opened the PR. As you suggested, it just adds testMaterializedViewPrivileges()

I can take a pass at testForeignTablePrivileges() as well. Just let me know if that will be in a new issue. If I can't figure it out then I'll have to leave for someone else.

davecramer added a commit that referenced this issue Nov 12, 2021
* test: add and drop a materialized view

Add to TestUtil and also to DatabaseMetaData setup and teardown

fixes #2060

* test: materialized view privileges

make "matviewtest" all lowercase so it can be found in pg catalog, which lowercases all names

fixes #2060

Co-authored-by: Dave Cramer <davecramer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants