-
Notifications
You must be signed in to change notification settings - Fork 547
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 pg_catalog.has_table_privilege function #15965
Conversation
51e79d2
to
b352520
Compare
A bug:
This causes test failures that were about to be added:
In order to fix the bug, we need to use update: these tests actually tests |
server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java
Outdated
Show resolved
Hide resolved
Could you review this PR @mfussenegger ?
|
* @param <R> the return type | ||
*/ | ||
@FunctionalInterface | ||
public interface FiveFunction<A, B, C, D, E, R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tend to instead create a concrete interface - getting the impression that this is taking the generic approach a bit too far.
|
||
public static final FunctionName NAME = new FunctionName(PgCatalogSchemaInfo.NAME, "has_table_privilege"); | ||
|
||
private static final FiveFunction<Roles, Role, Object, Collection<Permission>, Provider<Schemas>, Boolean> CHECK_BY_TABLE_NAME = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these regular static functions, and then use method refs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this has-privilege stuff is a bit odd given the mix of inheritance and composition. Wouldn't it be clearer to use either one approach instead of both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I thinkg that composition is the way to go, (taking into account those four/fiveFunction ifaces)
@@ -31,12 +33,23 @@ public class NodeContext { | |||
private final Functions functions; | |||
private final long serverStartTimeInMs; | |||
private final Roles roles; | |||
private final Provider<Schemas> schemasProvider; | |||
|
|||
public static NodeContext withoutSchemas(Functions functions, Roles roles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think allowing schemas to be absent will make it a lot harder to reason about - as you'll need to know what kind of NodeContext instance you've got, and if you can access the schema.
We could avoid this by by making it mandatory.
One way to do that would be to move away from guice for the Schemas creation, see de8ce16
Note that tests are broken on the commit, they fail with:
Could not find a suitable constructor in io.crate.metadata.Schemas. Classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private.
at io.crate.metadata.Schemas.class(Unknown Source)
at _unknown_
You'd also have to go through all classes that get Schemas
injected in the constructor and replace it with NodeContext
and lazy retrieve Schemas
on use.
An alternative approach could be to try to break the cyclic dependency, by removing/adapting the dependencies of the Schemas
class. E.g. maybe it could take Provider<DocSchemaInfoFactory>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, continuing on your first approach #15988.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tend to agree with @mfussenegger.
That we're currently using different NodeContext
instances already is in my opinion already a bad design, and instead of moving this further I'd prefer to get rid of this. This looks cumbersome to me, even when protecting fishy calls by the exception.
Related to the suggestions of getting rid of the Schema injections. Wouldn't it even possible to get rid of injection in the related classes at all instead of moving the resolving from directly binding Schemas to resolving it over the bound NodeContext? Not sure if this is feasible (related to time spent on this feature), but as I think our overall goal is in this direction, maybe worth doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -513,4 +515,32 @@ public boolean viewExists(RelationName relationName) { | |||
ViewsMetadata views = clusterService.state().metadata().custom(ViewsMetadata.TYPE); | |||
return views != null && views.getView(relationName) != null; | |||
} | |||
|
|||
public String oidToName(int oid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public String oidToName(int oid) { | |
@Nullable | |
public RelationName getRelation(int oid) { |
I'd make this specific to tables and return a RelationName
, not a string to make it more general useful.
The schema lookup is afaik currently also not needed?
be9361b
to
b849b7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @jeeminso!
I've left some comments and most importantly, to use the correct permission types for has_table_privilege
and not those for the schema.
Please also add docs and changelog entry.
server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/expression/scalar/HasPrivilegeFunction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java
Outdated
Show resolved
Hide resolved
Edge case:
|
Maybe we should check this on the |
On second thought, if we do that we are going to throw a different error message than we do now, when a user tries to insert/update/delete on those tables, so maybe we should only treat this for the |
We seem to have two concepts, operations and privileges, where the error messages you mentioned are from operation checks on the given table,
I think one way to handle this is to have the newly introduced Anyways, as you said this could be continued as a follow up. |
158f303
to
4c6486d
Compare
Thanks for reviewing @matriv please check again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! left some more minor comments.
server/src/test/java/io/crate/expression/scalar/HasTablePrivilegeFunctionTest.java
Show resolved
Hide resolved
server/src/test/java/io/crate/integrationtests/HasTablePrivilegeFunctionIntegrationTest.java
Show resolved
Hide resolved
In postgres if you grant the permission on a static table you get a
|
4c6486d
to
ed19d37
Compare
Summary of the changes / Why this improves CrateDB
Resolves #15748.
Initially discussed to go by extending NodeContext with ClusterState but since system tables are not available in cluster state, went by extending NodeContext with Schemas.
Checklist
docs/appendices/release-notes/<x.y.0>.rst
for user facing changessql_features
table for user facing changesdocs/appendices/release-notes/<x.y.0>.rst
(E.g. AdminUI)