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.
