db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-4272) SQL Authorization Support for dblook
Date Wed, 17 Jun 2009 15:06:07 GMT

    [ 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.


Mime
View raw message