[
https://issues.apache.org/jira/browse/DERBY-4272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12720701#action_12720701
]
Dag H. Wanvik commented on DERBY-4272:
--------------------------------------
I have been though the code in a first pass. Thanks for this great
work, Hiranya, it is definitely taking us a long step in the right
direction!
I'll post most my findings now (non-trivial and trivial),
I am sure you know about some of them
already, but any way... There is a lot of new code, so please forgive if
I have overlooked stuff :) Please don't get discouraged by all the
comments, this is really good progress!!
- Nice code for that general graph handling. I didn't quite see where
the fact that the objects are comparable (orderable) comes into
play, cf the comment in DBPersistentObject:
"Objects of higher levels of the dependency hierarchy should get
lesser type values".
There is also the explicit dependencies encoded in the edges, so my
question is, how is this ordering used? Is it supposed to be used when
walking the graph somehow?
- There is no encoding of the dependency a persistent object might
have on a role, and similarly no code for setting the role before
attempting its creation yet, right? I guess you just didn't get that
far yet :)
- Did you catch the dependency a CHECK constraint may have on
functions? I could not find that..
- Similarly, the GENERATED ALWAYS AS (expression) may cause dependency
on a function and I could not see that being captured.
- Views, triggers not handled yet. Both may have dependencies on other
objects as well as role.
- In DBPersistenObject.getDDL you use a switch to handle to object
subtypes. Why not use subclassing here?
The method name "getDDL" is a bit misleading to me, since it does
not return the DDL, it has a side effect: it write the DDL to Log.
Maybe this is the pattern used in dblook; I tend to prefer such
methods to format their output strings and return them and let the
upper layers handle the actual printing.. But it would be nice to
make the name more descriptive, maybe emitDDL or some such.
For "case DB_OBJECT_TYPE_FUNCTION", the "dumpPerms = true" is missing
I think.
- In DBPersistenObject.getPermissions, is GrantedPermissions.size()
ever 0? If not, I'd prefer an invariant guard instead leniency..
- In DiGraph.remove, does the case "e.end() == data" ever arise? I
thought edges would only ever be removed when the e.getStart !=
null.. (In a generic graph package the code is correct and necessary
of course)
- in DBPermission.getDDL, the parent object is an argument. Would it
perhaps be cleaner to pass that in in the constructor since along
with grantor and grantee?
In the DB_COLUMN_PERMISSION case, you use just the first ([0]) value
returned from getGrantedPermissions.. Not sure I understood why.. In
any case an explanation would be good here.
- In DB_Schema.doSchemas, it would be good to explain why you skip the
schema called "APP"? One could envision a user called "APP", so is
it always safe to skip it?
- In doTables, there is an "if (graph.vertex(currentTable) == null)".
When would it ever be != null? Perhaps when a dependency on that
table was already registered? Please add a comment..
- in DB_key.addConstraint, you do a call to getProviderType. Great
that you figured out how to handle this :)
But do you handle the case where the returned type is
DB_OBJECT_TYPE_TABLE?
Some minor nits which I'm sure you'll handle when the time comes
(added mainly here so you can see I have read the code ;-)
- We try to keep lines within 80 columns and not use TABs or blanks at end of
lines
in new code.
- Some magic numbers should be made into constants, e.g. positions in
result set from queries with "*" forces the reader to look up the
table definition see see whats being read, sa in a getString(3).
- missing Javadoc for classes and public members.
- typo or "vertex" in Javadoc for DiGraph.remove.
- In DB_Schema.doSchemas, in the if statement "if (tablesOnly | ..)"
you do a "continue". Wouldn't a "break" be better for the tablesOnly
case?
- in getTableOwner, I'd replace the "while" with an "if .. else" for
better legibility. Same for getKeyOwner and the "while
(authorResultSet.next()" loop in getVertices.
- in doKeys, the variable "getReferenceCols" is unused?
Thanks, and keep up the good work! I'll be out till Monday.
> SQL Authorization Support for dblook
> ------------------------------------
>
> Key: DERBY-4272
> URL: https://issues.apache.org/jira/browse/DERBY-4272
> Project: Derby
> Issue Type: Improvement
> Components: Tools
> Environment: Any
> Reporter: Hiranya Jayathilaka
> Attachments: DERBY-4272-changes-u1.txt, DERBY-4272-u1.patch
>
>
> Currently dblook suffers from two major shortcomings.
> 1. dblook doesn't take the object dependencies into consideration when
> generating DDL scripts
> 2. dblook doesn't have any support for SQL authorization
> I intend to fix these two issues and improve dblook so that the DDL scripts
> generated by dblook can be executed without errors under all conditions.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
