[
http://issues.apache.org/jira/browse/DERBY-1330?page=comments#action_12419836 ]
Daniel John Debrunner commented on DERBY-1330:
----------------------------------------------
What would be really helpful if was if this patch was split into multiple
patches.
I'm finally realizing the patch is making several changes:
1) Add mechanism to stop collection of privilege information during compilation
2) Use mechanism of 1) for view execution
3) Add a UUID to the existing privilege systems tables.
4) Change the PrivilegeDescriptor classes to be Providers as well as being
TupleDescriptors
5) Allowing GRANT to execute on views
6) Make CREATE VIEW depend on the(existing) list of priviliges collected
7) Stuff added for triggers added recently that I haven't managed to look at.
8) Stuff added for constraints added recently that I haven't managed to look at.
9) Others I've missed due to the size of the patch
+) Test changes.
So 8+) seperate changes "forced" into a single 352k patch. Personally I find
patches like this really hard to review due to their size and multiple issues.
I believe the risk of missing something in a review or in writing the code is
increased by the size of the change, thus the quality of Derby can be degraded
by accepting large patches. Incremental development is good.
I know it's hard to anticipate & identify incremental changes from the start of
implementing functionality, but it's a good habit to get into. As one
develops something and realise that a new piece of functionality is needed,
stop and think if it could be implemented in a separate clean code line and
delivered as a separate patch. For example, with this change the requirement
for the UUID to be added to the existing system tables could have been
identified as a stand-alone item.
Another technique I use is to separate items after the fact. Once a codeline
becomes clogged with multiple changes, I pull individual changes into a
separate code line and then test & submit incrementally. I will write this up
in the wiki.
Now, I do believe that the we, the community, could maybe do a better job to
encourage smaller patches, faster turn around on submitted small patches etc.,
but I also believe, in general, the contributors can help a lot here. At least
by following the guidelines from the wiki. I think we are in somewhat of a
negative-feedback situation, patches are slow to be reviewed & committed due to
time constraints and patch complexity leading to contributors trying to cram
more things into a single patch to only have to undergo a single commit leading
to longer review times etc. etc. If we could break the cycle and have fast
turnaround on small patches it would move more people to the incremental
development model.
As for this specific patch, I'm not sure it's a good plan to have you submit 8+
patches & the testing associated with it, given we have a working patch here.
There are some more changes I would like to see, and I would like you to
refrain from adding any more functionality into the patch. I wll add specific
review comments in another comment.
> Provide runtime privilege checking for grant/revoke functionality
> -----------------------------------------------------------------
>
> Key: DERBY-1330
> URL: http://issues.apache.org/jira/browse/DERBY-1330
> Project: Derby
> Type: Sub-task
> Components: SQL
> Versions: 10.2.0.0
> Reporter: Mamta A. Satoor
> Assignee: Mamta A. Satoor
> Attachments: AuthorizationModelForDerbySQLStandardAuthorization.html,
> AuthorizationModelForDerbySQLStandardAuthorizationV2.html,
> Derby1330PrivilegeCollectionV2diff.txt,
> Derby1330PrivilegeCollectionV2stat.txt,
> Derby1330ViewPrivilegeCollectionV1diff.txt,
> Derby1330ViewPrivilegeCollectionV1stat.txt
>
> Additional work needs to be done for grant/revoke to make sure that only
> users with required privileges can access various database objects. In order
> to do that, first we need to collect the privilege requirements for various
> database objects and store them in SYS.SYSREQUIREDPERM. Once we have this
> information then when a user tries to access an object, the required
> SYS.SYSREQUIREDPERM privileges for the object will be checked against the
> user privileges in SYS.SYSTABLEPERMS, SYS.SYSCOLPERMS and
> SYS.SYSROUTINEPERMS. The database object access will succeed only if the user
> has the necessary privileges.
> SYS.SYSTABLEPERMS, SYS.SYSCOLPERMS and SYS.SYSROUTINEPERMS are already
> populated by Satheesh's work on DERBY-464. But SYS.SYSREQUIREDPERM doesn't
> have any information in it at this point and hence no runtime privilege
> checking is getting done at this point.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
http://www.atlassian.com/software/jira
