db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel John Debrunner <...@apache.org>
Subject Re: Grant Revoke notes ....
Date Tue, 25 Jul 2006 20:50:50 GMT
Satheesh Bandaram wrote:

> Thanks for creating this GrantRevoke notes, Dan. Here are some comments
> on some of your points.
> 
>     http://wiki.apache.org/db-derby/GrantRevokeImplementation

Thanks for reading them, they were a dump of my thoughts are i had
looked at the code. I think with any project one can always come along
later and see potential improvements and/or simplifications, that's
really what most of my "issues" are. Issues were things that jumped out
at me as confusing, over-complicated, strange and/or potential for
future bugs.

> 
> 1) "PrivilegeInfo looks very much like a constant action, the constant
> action for grant/revoke is more or less just an empty object that calls
> the execute method on PrivilegeInfo. Should this code be re-factored to
> have all the logic in a constant action, to match the pattern of other
> DDL statements."
> 
> PrivilegeInfo and GrantRevokeConstantAction is modeled much like
> constraints are handled.... In both cases one parent statement could
> result in multiple different database objects being created. But this
> two levels is more needed for constraints as they can be created in
> multiple ways, than for privileges. It should be possible to refactor,
> but a GRANT or REVOKE action could be dealing with number of different
> types of privileges and already deals with two now. (TablePrivilegeInfo
> and RoutinePrivilegeInfo) It is likely that GRANT and REVOKE would deal
> with more types of privileges in the future and this design is more
> object oriented. But does result in extra classes, so we have to decide
> if overhead is worth it or not.

I was thinking of a GrantConstantAction and a RevokeConstantAction and
their input being based upon StatementPermission objects. Replacing the
 PrivilegeInfo hierarchy with the StatementPermission one. Currently if
I wanted to add a new Permission I have to add it in two places
PrivilegeInfo and StatementPermission where it seems one would suffice.

> 2) "PermissionsDescriptor object passed in is reset each time with a
> different grantee - does this match the intended purpose of such
> descriptors which are TupleDescriptors and match a single row in the
> database?"
> 
> This can be changed.. probably should be.
> 
> 3) "Too many ways of representing permissions, PrivilegeInfo,
> StatementPermission, PermissionDescriptor, UUID. Possible solution"
> 
> PermissionDescriptors are more like TableDescriptors... represent a
> granted privilege present in system tables. Currently there are
> TablePermDescriptors, RoutinePermDescriptors and  ColPermDescriptors,
> representing each of three new system catalogs. (RequiredPermDescriptor
> would go away) StatementPermission represents required database object
> access that need to be verified for permission at runtime. Note that
> these access descriptors could be for objects that don't have
> PermissionDescriptors... like StatementSchemaPermission object and also
> don't have fields like grantor/grantee/grantWithGrant etc. There is no
> 1-1 mapping in their purpose, I think, but some refactoring may be
> possible. Result could turn out be as messier if not more, but might
> reduce some footprint.

I think it would be cleaner if we had StatementPermission &
PermissionsDescriptor as the two hierarchies, and clean separation of
logic between the two. PermissionsDescriptor would just represent the
rows in the system catalogs, StatementPermission used for runtime &
compile time. I even thought that possibly PermissionsDescriptor could
return a StatementPermission rather than duplicate its methods.


> 
> 4) Items about HashMaps/HashSet...
> 
> Possibly some improvement can be made here, but nothing seems incorrect.
> Hashing is done to make sure only one StatementPermission object is
> created for a given type of access on same database object. Otherwise,
> we could create multiple access checks and perform same privilege
> checking multiple times.

Probably not incorrect, but having multiple ways to handle the same
basic operation looked really strange to me, why are routine permissions
only stored with the UUID of the routine, not a
StatementRoutinePermission? In my mind a more consistent clean approach
would be to have one hashtable, key being the UUID of the object the
permission is on, the value being a StatementPermission. Then maybe a
new method on StatementPermission, addToHash(HashMap map) that performs
any lookup logic to keep that logic within the StatementPermission
sub-class, rather than separate in CompilerContextImpl. Then ideally to
add a new permission one simply adds the StatementPermission sub-class
and all the general framework code just works. Currently if I add a new
permission I will have to look for everywhere I need to change.

> Guess the *main *concern is multiple objects being created and see if
> they can be refactored, right?

No sure if this was an overal comment, or just for 4), but that's not my
main concern.

Dan.


Mime
View raw message