Derby
  1. Derby
  2. DERBY-4272

SQL Authorization Support for dblook

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Tools
    • Labels:
      None
    • Environment:
      Any

      Description

      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.

      1. old.sql
        4 kB
        Hiranya Jayathilaka
      2. new.sql
        6 kB
        Hiranya Jayathilaka
      3. dhw-sample-1.sql
        2 kB
        Dag H. Wanvik
      4. DERBY-4272-u6.patch
        129 kB
        Hiranya Jayathilaka
      5. DERBY-4272-u5.patch
        108 kB
        Hiranya Jayathilaka
      6. DERBY-4272-u4.patch
        108 kB
        Hiranya Jayathilaka
      7. DERBY-4272-u3.patch
        91 kB
        Hiranya Jayathilaka
      8. DERBY-4272-u2.patch
        55 kB
        Hiranya Jayathilaka
      9. DERBY-4272-u1.patch
        46 kB
        Hiranya Jayathilaka
      10. DERBY-4272-changes-u6.txt
        2 kB
        Hiranya Jayathilaka
      11. DERBY-4272-changes-u5.txt
        2 kB
        Hiranya Jayathilaka
      12. DERBY-4272-changes-u4.txt
        2 kB
        Hiranya Jayathilaka
      13. DERBY-4272-changes-u3.txt
        2 kB
        Hiranya Jayathilaka
      14. DERBY-4272-changes-u2.txt
        1 kB
        Hiranya Jayathilaka
      15. DERBY-4272-changes-u1.txt
        0.8 kB
        Hiranya Jayathilaka

        Issue Links

          Activity

          Show
          Hiranya Jayathilaka added a comment - Please find the detailed proposal at http://wiki.apache.org/general/HiranyaJayathilaka/gsoc2009/derby-dblook-proposal
          Hide
          Hiranya Jayathilaka added a comment -

          Attaching first patch of this effort for review and feedback.

          Show
          Hiranya Jayathilaka added a comment - Attaching first patch of this effort for review and feedback.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the patch, Hiranya. Looking at it now; will be back with comments shortly.

          Show
          Dag H. Wanvik added a comment - Thanks for the patch, Hiranya. Looking at it now; will be back with comments shortly.
          Hide
          Dag H. Wanvik added a comment -

          It would be good to have a pointer back to the design page
          at http://wiki.apache.org/db-derby/sqlAuth4Dblook?highlight=%28dblook%29 in the description.

          Show
          Dag H. Wanvik added a comment - It would be good to have a pointer back to the design page at http://wiki.apache.org/db-derby/sqlAuth4Dblook?highlight=%28dblook%29 in the description.
          Hide
          Dag H. Wanvik added a comment - - edited

          A preliminary comment: I notice is that it does the patch does not compile using ant because you have used Java 1.5 features (generics). The Derby code base is based on Java 1.4 source. The reason is that the code should be able to run on older VMs.

          Show
          Dag H. Wanvik added a comment - - edited A preliminary comment: I notice is that it does the patch does not compile using ant because you have used Java 1.5 features (generics). The Derby code base is based on Java 1.4 source. The reason is that the code should be able to run on older VMs.
          Hide
          Hiranya Jayathilaka added a comment -

          My plan was to make the DiGraph class a general data structure which can be used to store any kind of data through generics. That way we could have used it to store the dependency graph as well as the role dependency graph (and any other directed graph we'll ever need). Anyway if generics cannot be used I'll make this class specific to the DBPersistentObject type. That will not alter the existing functionality but for the role dependency graph we'll have to write another data structure in the near future.

          I'll send in a new patch soon.

          Show
          Hiranya Jayathilaka added a comment - My plan was to make the DiGraph class a general data structure which can be used to store any kind of data through generics. That way we could have used it to store the dependency graph as well as the role dependency graph (and any other directed graph we'll ever need). Anyway if generics cannot be used I'll make this class specific to the DBPersistentObject type. That will not alter the existing functionality but for the role dependency graph we'll have to write another data structure in the near future. I'll send in a new patch soon.
          Hide
          Dag H. Wanvik added a comment -

          I guess you could still use one common class, at the expense of type safety (you'd need to use Object to cover for both DBPersistentObject and the role object. Too bad you can't use generics

          Show
          Dag H. Wanvik added a comment - I guess you could still use one common class, at the expense of type safety (you'd need to use Object to cover for both DBPersistentObject and the role object. Too bad you can't use generics
          Hide
          Dag H. Wanvik added a comment -

          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.

          Show
          Dag H. Wanvik added a comment - 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.
          Hide
          Hiranya Jayathilaka added a comment -

          Made changes based on the feedback by Dag. Summary of changes:

          • Got rid of generics
          • Used subclassing to handle DDL generation for different types of objects
          • Minor bug fixes
          • Added support for handling check constraints along with their dependencies to aliases
          • General refactoring and code cleanup work (some more left - will address in the next patch)

          Thanks Dag for the great feedback provided.

          Show
          Hiranya Jayathilaka added a comment - Made changes based on the feedback by Dag. Summary of changes: Got rid of generics Used subclassing to handle DDL generation for different types of objects Minor bug fixes Added support for handling check constraints along with their dependencies to aliases General refactoring and code cleanup work (some more left - will address in the next patch) Thanks Dag for the great feedback provided.
          Hide
          Dag H. Wanvik added a comment -

          Re-posting this mail from Hiranya with answers and comments here to
          have all info in one place (common Derby practice

          Mail to derby-dev on Thu, 18 Jun 2009 11:22:33 +0530.
          ----------------------------------------------------------------------------
          Hi Dag,
          Thanks a lot for reviewing the patch and for the detailed analysis provided.
          I decided to reply to the notification mail rather than to the JIRA entry
          since I can better correlate my answers with your questions.

          Please see my comments in-line.

          > 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!

          Thanks. This is really encouraging.

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

          The object comparison logic is in the DBObjectComparator class which
          implements the standard Comparator interface. It uses the type values of the
          DBPersistentObject instances to prioritize them accordingly during DDL
          generation. (Currently I have used some generics there. Will get rid of
          that)

          > 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?

          The edge direction is used to identify the provider and the dependent of a
          dependency relationship. This directionality information is used during the
          graph walk as you have mentioned to locate objects which doesn't have any
          providers associated with them. See the DiGraph#getRoot method.

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

          Yes we need to implement this. In fact I was going to start a discussion on
          this on the mailing list soon

          > - Did you catch the dependency a CHECK constraint may have on
          > functions? I could not find that..

          I haven't implemented DDL generation for CHECK constraints yet. It's only
          implemented for primary key, foreign key and unique constraints yet. Will
          look into this.

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

          Yes this is for the future.

          > - In DBPersistenObject.getDDL you use a switch to handle to object
          > subtypes. Why not use subclassing here?

          This is also an option. Will look into this. The only issue I see in this
          approach is that we have to write a class for each persistent object type.
          Currently we handle all objects with one class.

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

          OK I'll perform the necessary refactoring on the code.

          > For "case DB_OBJECT_TYPE_FUNCTION", the "dumpPerms = true" is missing
          > I think.

          Bug! Will fix this up for the next patch.

          > - In DBPersistenObject.getPermissions, is GrantedPermissions.size()
          > ever 0? If not, I'd prefer an invariant guard instead leniency..

          True. Since we don't allow removing permissions from the objects the size
          can never be '0'. The list is either null or populated with at least one
          permission. Thanks for pointing this out. I'll remove the redundant check.

          > - 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)

          Well because of the way we use the graph this condition will never arise.
          But in a more general sense, as you also have pointed out, I think this is
          the correct behavior. Otherwise there is chance that an edge can hang around
          without an ending vertex. So I prefer to leave this logic as it is.
          Otherwise the graph implementation doesn't really comply with the
          mathematical definition of 'graph'.

          > - 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?

          +1. We should be able to do that.

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

          Looks like a bug. I'll fix this to iterate over all granted permissions.

          > - 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?

          I think the old doSchemas method does the same. I just took the logic from
          there. I noticed that when the "APP" schema is not skipped a bunch of
          unnecessary schemas get included in the graph. So I guess the correct
          behavior is 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..

          That's the idea. But because we tackle this task level-by-level (ie we first
          handle schemas, then functions/procedures, then tables and so on) this check
          is actually redundant. We should be able to get rid of it safely.

          > - 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?

          No we don't have to. By the time we handle constraints, tables are already
          handled and they are available in the graph (because we handle one level at
          a time). So provider table objects will be always found from the graph and
          so we just need to create an edge. Just to further clarify the things, I'm
          following the dependency ordering pointed out by Rick in a previous thread
          to during graph creation. As Rick pointed out by then we don't have to worry
          about inter-level dependencies, we just need to handle intra-level
          dependencies.

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

          My plan is to get rid of '*' queries at the end of the project. Currently
          I'm trying out lot of things with the code so it's easier to have '*'
          queries.

          > - missing Javadoc for classes and public members.

          Absolutely +1. I'll work on that

          > - typo or "vertex" in Javadoc for DiGraph.remove.

          You really have gone through every single line of code Thanks for
          pointing this out.

          > - In DB_Schema.doSchemas, in the if statement "if (tablesOnly | ..)"
          > you do a "continue". Wouldn't a "break" be better for the tablesOnly
          > case?

          +1

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

          That makes sense. Since there is only record in the result set, right?

          > - in doKeys, the variable "getReferenceCols" is unused?

          Correct. Let's get rid of that.

          > Thanks, and keep up the good work!

          Indeed. And thanks again for the great analysis. Expect another patch by
          next weekend.

          Show
          Dag H. Wanvik added a comment - Re-posting this mail from Hiranya with answers and comments here to have all info in one place (common Derby practice Mail to derby-dev on Thu, 18 Jun 2009 11:22:33 +0530. ---------------------------------------------------------------------------- Hi Dag, Thanks a lot for reviewing the patch and for the detailed analysis provided. I decided to reply to the notification mail rather than to the JIRA entry since I can better correlate my answers with your questions. Please see my comments in-line. > 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! Thanks. This is really encouraging. > 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". The object comparison logic is in the DBObjectComparator class which implements the standard Comparator interface. It uses the type values of the DBPersistentObject instances to prioritize them accordingly during DDL generation. (Currently I have used some generics there. Will get rid of that) > 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? The edge direction is used to identify the provider and the dependent of a dependency relationship. This directionality information is used during the graph walk as you have mentioned to locate objects which doesn't have any providers associated with them. See the DiGraph#getRoot method. > - 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 Yes we need to implement this. In fact I was going to start a discussion on this on the mailing list soon > - Did you catch the dependency a CHECK constraint may have on > functions? I could not find that.. I haven't implemented DDL generation for CHECK constraints yet. It's only implemented for primary key, foreign key and unique constraints yet. Will look into this. > - 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. Yes this is for the future. > - In DBPersistenObject.getDDL you use a switch to handle to object > subtypes. Why not use subclassing here? This is also an option. Will look into this. The only issue I see in this approach is that we have to write a class for each persistent object type. Currently we handle all objects with one class. > 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. OK I'll perform the necessary refactoring on the code. > For "case DB_OBJECT_TYPE_FUNCTION", the "dumpPerms = true" is missing > I think. Bug! Will fix this up for the next patch. > - In DBPersistenObject.getPermissions, is GrantedPermissions.size() > ever 0? If not, I'd prefer an invariant guard instead leniency.. True. Since we don't allow removing permissions from the objects the size can never be '0'. The list is either null or populated with at least one permission. Thanks for pointing this out. I'll remove the redundant check. > - 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) Well because of the way we use the graph this condition will never arise. But in a more general sense, as you also have pointed out, I think this is the correct behavior. Otherwise there is chance that an edge can hang around without an ending vertex. So I prefer to leave this logic as it is. Otherwise the graph implementation doesn't really comply with the mathematical definition of 'graph'. > - 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? +1. We should be able to do that. > 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. Looks like a bug. I'll fix this to iterate over all granted permissions. > - 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? I think the old doSchemas method does the same. I just took the logic from there. I noticed that when the "APP" schema is not skipped a bunch of unnecessary schemas get included in the graph. So I guess the correct behavior is 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.. That's the idea. But because we tackle this task level-by-level (ie we first handle schemas, then functions/procedures, then tables and so on) this check is actually redundant. We should be able to get rid of it safely. > - 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? No we don't have to. By the time we handle constraints, tables are already handled and they are available in the graph (because we handle one level at a time). So provider table objects will be always found from the graph and so we just need to create an edge. Just to further clarify the things, I'm following the dependency ordering pointed out by Rick in a previous thread to during graph creation. As Rick pointed out by then we don't have to worry about inter-level dependencies, we just need to handle intra-level dependencies. > 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). My plan is to get rid of '*' queries at the end of the project. Currently I'm trying out lot of things with the code so it's easier to have '*' queries. > - missing Javadoc for classes and public members. Absolutely +1. I'll work on that > - typo or "vertex" in Javadoc for DiGraph.remove. You really have gone through every single line of code Thanks for pointing this out. > - In DB_Schema.doSchemas, in the if statement "if (tablesOnly | ..)" > you do a "continue". Wouldn't a "break" be better for the tablesOnly > case? +1 > - 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. That makes sense. Since there is only record in the result set, right? > - in doKeys, the variable "getReferenceCols" is unused? Correct. Let's get rid of that. > Thanks, and keep up the good work! Indeed. And thanks again for the great analysis. Expect another patch by next weekend.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for thew new patch, Hiranya!

          A small comment to your answer to the previous patch:

          > I think the old doSchemas method does the same. I just took the logic from
          > there. I noticed that when the "APP" schema is not skipped a bunch of
          > unnecessary schemas get included in the graph. So I guess the correct
          > behavior is to skip it?

          I think the schema creation is skipped in the old code due to the fact
          that the user's schema will be automatically created the first time a
          database object is created (and no explicit schema is given), and when
          authentication is not enabled, the default schema will be APP.

          However, this auto-create function applies also when authentication is
          enabled. So, when authentication on, the "skipping" should, if we
          decide to not create the schemas explicitly, apply to all user
          schemas. I think we might as well create the schemas explicitly for
          clarity.

          Show
          Dag H. Wanvik added a comment - Thanks for thew new patch, Hiranya! A small comment to your answer to the previous patch: > I think the old doSchemas method does the same. I just took the logic from > there. I noticed that when the "APP" schema is not skipped a bunch of > unnecessary schemas get included in the graph. So I guess the correct > behavior is to skip it? I think the schema creation is skipped in the old code due to the fact that the user's schema will be automatically created the first time a database object is created (and no explicit schema is given), and when authentication is not enabled, the default schema will be APP. However, this auto-create function applies also when authentication is enabled. So, when authentication on, the "skipping" should, if we decide to not create the schemas explicitly, apply to all user schemas. I think we might as well create the schemas explicitly for clarity.
          Hide
          Hiranya Jayathilaka added a comment -
          • Implemented dependency graph based DDL generation for jar files, views, triggers and indexes
          • Minor code refactoring and cleanup work based on the feedback from the community
          • Implemented a switch to change the behavior of SQL CONNECT statement generation (by default such statements will be commented out - use connectStmt argument to change this behavior)

          With this patch we can generate a DDL for a database without roles support. My next patch will focus on getting in roles support based on the dependency graph.

          As always I'd appreciate any feedback on this patch

          Show
          Hiranya Jayathilaka added a comment - Implemented dependency graph based DDL generation for jar files, views, triggers and indexes Minor code refactoring and cleanup work based on the feedback from the community Implemented a switch to change the behavior of SQL CONNECT statement generation (by default such statements will be commented out - use connectStmt argument to change this behavior) With this patch we can generate a DDL for a database without roles support. My next patch will focus on getting in roles support based on the dependency graph. As always I'd appreciate any feedback on this patch
          Hide
          Hiranya Jayathilaka added a comment -

          Attaching feature complete patch. Changes made since the last patch:

          • Implemented support for creating and granting roles
          • Implemented support for setting the necessary roles before constructing each object
          • Implemented support for synonyms
          • Implemented support for handling view permission grants
          • Changed the DDL generation logic foe views so that it would use full qualified view names in the CREATE statements - With this we no longer need to explicitly set the schema before creating a view
          • Added comments and javadocs
          • Minor code refactoring and cleanup work
          Show
          Hiranya Jayathilaka added a comment - Attaching feature complete patch. Changes made since the last patch: Implemented support for creating and granting roles Implemented support for setting the necessary roles before constructing each object Implemented support for synonyms Implemented support for handling view permission grants Changed the DDL generation logic foe views so that it would use full qualified view names in the CREATE statements - With this we no longer need to explicitly set the schema before creating a view Added comments and javadocs Minor code refactoring and cleanup work
          Hide
          Hiranya Jayathilaka added a comment -

          Attaching two DDL files generated by using the existing dblook implementation (old.sql) and improved dblook implementation (new.sql) for some early feedback. Both scripts are against the same database.

          Note how the new dblook implementation switches between multiple database connections (CONNECT statements are printed as comments - this behavior isconfigurable through a command line argument) when constructing objects.

          Also note how it sets the roles before constructing each object. The dependencies among objects are also preserved. Permission grants take place interleaved with object creation.

          The database used for DDL generation does not have too many objects. Will provide some more sample scripts for a database having a large number of objects.

          Show
          Hiranya Jayathilaka added a comment - Attaching two DDL files generated by using the existing dblook implementation (old.sql) and improved dblook implementation (new.sql) for some early feedback. Both scripts are against the same database. Note how the new dblook implementation switches between multiple database connections (CONNECT statements are printed as comments - this behavior isconfigurable through a command line argument) when constructing objects. Also note how it sets the roles before constructing each object. The dependencies among objects are also preserved. Permission grants take place interleaved with object creation. The database used for DDL generation does not have too many objects. Will provide some more sample scripts for a database having a large number of objects.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the new patch, Hiranya! Great work!
          I will start looking at it closely asap.

          Show
          Dag H. Wanvik added a comment - Thanks for the new patch, Hiranya! Great work! I will start looking at it closely asap.
          Hide
          Dag H. Wanvik added a comment -

          I can't get the patch to compile in my sandbox (u4) without tweaking. It seems there are still Java 1.5 constructs being used.
          I see the build.xml file in java/tools/org/apache/derby/impl/tools now specifies source level 1.5, but we need to move that back to 1.4.
          But even with that setting it does not compile in my default sandbox (I run ant from the top level):

          $ ant -q all
          [javac] /export/home/tmp/derby/sb/sb4/java/tools/org/apache/derby/impl/tools/dblook/graphs/DBPersistentObject.java:92: cannot find symbol
          [javac] symbol : class Override
          [javac] location: class org.apache.derby.impl.tools.dblook.graphs.DBPersistentObject
          [javac] @Override

          Show
          Dag H. Wanvik added a comment - I can't get the patch to compile in my sandbox (u4) without tweaking. It seems there are still Java 1.5 constructs being used. I see the build.xml file in java/tools/org/apache/derby/impl/tools now specifies source level 1.5, but we need to move that back to 1.4. But even with that setting it does not compile in my default sandbox (I run ant from the top level): $ ant -q all [javac] /export/home/tmp/derby/sb/sb4/java/tools/org/apache/derby/impl/tools/dblook/graphs/DBPersistentObject.java:92: cannot find symbol [javac] symbol : class Override [javac] location: class org.apache.derby.impl.tools.dblook.graphs.DBPersistentObject [javac] @Override
          Hide
          Dag H. Wanvik added a comment -

          I had to change til line in java/tools/org/apache/derby/impl/tools/build.xml:

          <pathelement path="$

          {java14compile.classpath}

          "/>
          to
          <pathelement path="$

          {java15compile.classpath}

          "/>

          so be able to build.

          Show
          Dag H. Wanvik added a comment - I had to change til line in java/tools/org/apache/derby/impl/tools/build.xml: <pathelement path="$ {java14compile.classpath} "/> to <pathelement path="$ {java15compile.classpath} "/> so be able to build.
          Hide
          Dag H. Wanvik added a comment -

          Did a trial run of a database with two users, authentication/authorization set, and could see the DDL
          containing session switching + proper role creation/setting needed reconstruct a view on the other user's table. Very cool!

          Show
          Dag H. Wanvik added a comment - Did a trial run of a database with two users, authentication/authorization set, and could see the DDL containing session switching + proper role creation/setting needed reconstruct a view on the other user's table. Very cool!
          Hide
          Dag H. Wanvik added a comment - - edited

          The resulting script (dag-sample-1.sql) has some issues:

          • The role creation can not occur before the DBO has made a connection.
          • SET ROLE "NONE" is not valid, it should be SET ROLE NONE (without
            the text quotes). It is an identifier, not a string.
          • The commented out statements lacked a final semicolon - it would be
            nice to add that so the user can just uncomment the connect
            statements.
          • The APP schema did not have any objects in it, so it might be
            skipped. Derby auto-creates schemas anyway, so in general there is
            no need to create empty schemas. With authentication, in my case
            there was no defined user called "APP", so I had to remove that part
            of the script to make it run.

          I am actually thinking now that the uncommenting serves little
          purpose.. Maybe we might as well output those statements also and let
          dblook issue a warning on the console that the script does not contain
          passwords?

          Show
          Dag H. Wanvik added a comment - - edited The resulting script (dag-sample-1.sql) has some issues: The role creation can not occur before the DBO has made a connection. SET ROLE "NONE" is not valid, it should be SET ROLE NONE (without the text quotes). It is an identifier, not a string. The commented out statements lacked a final semicolon - it would be nice to add that so the user can just uncomment the connect statements. The APP schema did not have any objects in it, so it might be skipped. Derby auto-creates schemas anyway, so in general there is no need to create empty schemas. With authentication, in my case there was no defined user called "APP", so I had to remove that part of the script to make it run. I am actually thinking now that the uncommenting serves little purpose.. Maybe we might as well output those statements also and let dblook issue a warning on the console that the script does not contain passwords?
          Hide
          Dag H. Wanvik added a comment -

          Maybe the DBO's connect statement should have a "create=true" attribute as well.

          Show
          Dag H. Wanvik added a comment - Maybe the DBO's connect statement should have a "create=true" attribute as well.
          Hide
          Hiranya Jayathilaka added a comment -

          $ ant -q all
          [javac] /export/home/tmp/derby/sb/sb4/java/tools/org/apache/derby/impl/tools/dblook/graphs/DBPersistentObject.java:92: cannot find symbol
          [javac] symbol : class Override
          [javac] location: class org.apache.derby.impl.tools.dblook.graphs.DBPersistentObject
          [javac] @Override

          Looks like Java 1.4 doesn't like the annotations like @Override. Will get rid of that.

          Show
          Hiranya Jayathilaka added a comment - $ ant -q all [javac] /export/home/tmp/derby/sb/sb4/java/tools/org/apache/derby/impl/tools/dblook/graphs/DBPersistentObject.java:92: cannot find symbol [javac] symbol : class Override [javac] location: class org.apache.derby.impl.tools.dblook.graphs.DBPersistentObject [javac] @Override Looks like Java 1.4 doesn't like the annotations like @Override. Will get rid of that.
          Hide
          Hiranya Jayathilaka added a comment -

          The resulting script (dag-sample-1.sql) has some issues:

          • The role creation can not occur before the DBO has made a connection.

          Hiranya: OK. I think I can use the arguments that we pass into dblook to generate a CONNECT statement for DBO

          • SET ROLE "NONE" is not valid, it should be SET ROLE NONE (without
            the text quotes). It is an identifier, not a string.

          Hiranya: This requires a trivial fix. Will carry out immediately.

          • The commented out statements lacked a final semicolon - it would be
            nice to add that so the user can just uncomment the connect
            statements.

          Hiranya: Sure thing. Let's add it.

          • The APP schema did not have any objects in it, so it might be
            skipped. Derby auto-creates schemas anyway, so in general there is
            no need to create empty schemas. With authentication, in my case
            there was no defined user called "APP", so I had to remove that part
            of the script to make it run.

          Hiranya: OK. So then guess I'll prevent the APP schema from being generated.

          I am actually thinking now that the uncommenting serves little
          purpose.. Maybe we might as well output those statements also and let
          dblook issue a warning on the console that the script does not contain
          passwords?

          Hiranya: I think it is required to maintain some backward compatibility? (as discussed on the mailing list). It was mentioned that without the CONNECT statements commented out, we cannot directly send the output of dblook to ij using pipes.

          Hiranya: All in all, thanks a lot for the great feedback Dag. I'll make the suggested changes and send in another patch over the weekend.

          Show
          Hiranya Jayathilaka added a comment - The resulting script (dag-sample-1.sql) has some issues: The role creation can not occur before the DBO has made a connection. Hiranya: OK. I think I can use the arguments that we pass into dblook to generate a CONNECT statement for DBO SET ROLE "NONE" is not valid, it should be SET ROLE NONE (without the text quotes). It is an identifier, not a string. Hiranya: This requires a trivial fix. Will carry out immediately. The commented out statements lacked a final semicolon - it would be nice to add that so the user can just uncomment the connect statements. Hiranya: Sure thing. Let's add it. The APP schema did not have any objects in it, so it might be skipped. Derby auto-creates schemas anyway, so in general there is no need to create empty schemas. With authentication, in my case there was no defined user called "APP", so I had to remove that part of the script to make it run. Hiranya: OK. So then guess I'll prevent the APP schema from being generated. I am actually thinking now that the uncommenting serves little purpose.. Maybe we might as well output those statements also and let dblook issue a warning on the console that the script does not contain passwords? Hiranya: I think it is required to maintain some backward compatibility? (as discussed on the mailing list). It was mentioned that without the CONNECT statements commented out, we cannot directly send the output of dblook to ij using pipes. Hiranya: All in all, thanks a lot for the great feedback Dag. I'll make the suggested changes and send in another patch over the weekend.
          Hide
          Dag H. Wanvik added a comment -

          > I think it is required to maintain some backward compatibility? (as
          > discussed on the mailing list). It was mentioned that without the
          > CONNECT statements commented out, we cannot directly send the output
          > of dblook to ij using pipes.

          Yes backward compatibility it is a concern, but for the case when
          authentication[1] and SQLAuthorization[2] is enabled, dblook hasn't
          really been working at all till now, so I think we are free to choose
          what to do for that case.

          Without authentication and SQLAuthorization enabled, there is really
          no point in creating objects from more than one session anyway; the db object's schema
          is all that matters, since all connected users can access all objects.

          Dag

          [1] derby.connection.requireAuthentication=true
          http://db.apache.org/derby/docs/dev/devguide/cdevcsecure42374.html

          [2] derby.database.SQLAuthorization=true
          http://db.apache.org/derby/docs/dev/devguide/cdevcsecure36595.html

          Show
          Dag H. Wanvik added a comment - > I think it is required to maintain some backward compatibility? (as > discussed on the mailing list). It was mentioned that without the > CONNECT statements commented out, we cannot directly send the output > of dblook to ij using pipes. Yes backward compatibility it is a concern, but for the case when authentication [1] and SQLAuthorization [2] is enabled, dblook hasn't really been working at all till now, so I think we are free to choose what to do for that case. Without authentication and SQLAuthorization enabled, there is really no point in creating objects from more than one session anyway; the db object's schema is all that matters, since all connected users can access all objects. Dag [1] derby.connection.requireAuthentication=true http://db.apache.org/derby/docs/dev/devguide/cdevcsecure42374.html [2] derby.database.SQLAuthorization=true http://db.apache.org/derby/docs/dev/devguide/cdevcsecure36595.html
          Hide
          Hiranya Jayathilaka added a comment -

          BTW you can get the CONNECT statements printed as regular DDL statements (uncommented) by passing the -connectStmt argument to dblook.

          Show
          Hiranya Jayathilaka added a comment - BTW you can get the CONNECT statements printed as regular DDL statements (uncommented) by passing the -connectStmt argument to dblook.
          Hide
          Hiranya Jayathilaka added a comment -
          • Removed Java 5 annotations
          • Made some minor changes to the code to get it compiled on JDK 1.4 (eg: using Integer objects when dealing with maps instead of int)
          • APP schema is now ignored when creating schemas
          • SET ROLE NONE statements are printed correctly
          • The DBO will create a connection before creating the roles
          • Minor changes to DDL generation logic (formatting changes)

          To get the DBO username I used conn.getMetaData().getUserName(). But I think this will work properly only if we start dblook with a connection belonging to DBO. Is that ok? If not what is the right way to find the DBO username?

          On a different note do we really need to create a connection (in the generated script) as DBO before creating roles? In order to run the generated script we need to create a connection anyway.

          The test case is coming along nicely. I may have some question on that. Will start a mail thread regarding them soon.

          Show
          Hiranya Jayathilaka added a comment - Removed Java 5 annotations Made some minor changes to the code to get it compiled on JDK 1.4 (eg: using Integer objects when dealing with maps instead of int) APP schema is now ignored when creating schemas SET ROLE NONE statements are printed correctly The DBO will create a connection before creating the roles Minor changes to DDL generation logic (formatting changes) To get the DBO username I used conn.getMetaData().getUserName(). But I think this will work properly only if we start dblook with a connection belonging to DBO. Is that ok? If not what is the right way to find the DBO username? On a different note do we really need to create a connection (in the generated script) as DBO before creating roles? In order to run the generated script we need to create a connection anyway. The test case is coming along nicely. I may have some question on that. Will start a mail thread regarding them soon.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the new patch. Will look at it more tomorrow. Answers to your questions:

          > * APP schema is now ignored when creating schemas

          Hopefully only if it is empty

          > To get the DBO username I used conn.getMetaData().getUserName(). But
          > I think this will work properly only if we start dblook with a
          > connection belonging to DBO. Is that ok? If not what is the right
          > way to find the DBO username?

          The safe way is to look for the owner of the system tables,
          e.g. SYS.SYSSCHEMAS.

          > On a different note do we really need to create a connection (in
          > the generated script) as DBO before creating roles? In order to run
          > the generated script we need to create a connection anyway.

          One can specify the user and password on the ij command line [1], yes,
          if that is what you mean (although it is not required).

          I guess that is OK sometimes, although it is generally deemed safer
          not to supply user credentials on the command line, since the
          strings can be gleaned by another process under UNIX, e.g. with
          ps(1). Editing the password into dblook output file avoids that
          security issue. So for the latter use case, it would be good to
          generate the DBO connect statement, I think.

          [1]
          http://db.apache.org/derby/docs/10.5/tools/rtoolsijproprefuser.html
          http://db.apache.org/derby/docs/10.5/tools/rtoolsijproprefpassword.html

          > The test case is coming along nicely. I may have some question on
          > that. Will start a mail thread regarding them soon.

          Great!

          Show
          Dag H. Wanvik added a comment - Thanks for the new patch. Will look at it more tomorrow. Answers to your questions: > * APP schema is now ignored when creating schemas Hopefully only if it is empty > To get the DBO username I used conn.getMetaData().getUserName(). But > I think this will work properly only if we start dblook with a > connection belonging to DBO. Is that ok? If not what is the right > way to find the DBO username? The safe way is to look for the owner of the system tables, e.g. SYS.SYSSCHEMAS. > On a different note do we really need to create a connection (in > the generated script) as DBO before creating roles? In order to run > the generated script we need to create a connection anyway. One can specify the user and password on the ij command line [1] , yes, if that is what you mean (although it is not required). I guess that is OK sometimes, although it is generally deemed safer not to supply user credentials on the command line, since the strings can be gleaned by another process under UNIX, e.g. with ps(1). Editing the password into dblook output file avoids that security issue. So for the latter use case, it would be good to generate the DBO connect statement, I think. [1] http://db.apache.org/derby/docs/10.5/tools/rtoolsijproprefuser.html http://db.apache.org/derby/docs/10.5/tools/rtoolsijproprefpassword.html > The test case is coming along nicely. I may have some question on > that. Will start a mail thread regarding them soon. Great!
          Hide
          Dag H. Wanvik added a comment -

          > BTW you can get the CONNECT statements printed as regular DDL
          > statements (uncommented) by passing the -connectStmt argument to
          > dblook.

          Good! Does that make the SET CONNECTION statements uncommented as
          well?

          Show
          Dag H. Wanvik added a comment - > BTW you can get the CONNECT statements printed as regular DDL > statements (uncommented) by passing the -connectStmt argument to > dblook. Good! Does that make the SET CONNECTION statements uncommented as well?
          Hide
          Hiranya Jayathilaka added a comment -

          > Does that make the SET CONNECTION statements uncommented as
          > well?

          Yes. SET CONNECTION statements are uncommented as well.

          Show
          Hiranya Jayathilaka added a comment - > Does that make the SET CONNECTION statements uncommented as > well? Yes. SET CONNECTION statements are uncommented as well.
          Hide
          Hiranya Jayathilaka added a comment -

          Uploading the patch for all my workings so far. Includes:

          • Complete source code including some of the recent fixes I have done to the code for the bugs discovered during testing
          • Partially completed test cases and related resources
          Show
          Hiranya Jayathilaka added a comment - Uploading the patch for all my workings so far. Includes: Complete source code including some of the recent fixes I have done to the code for the bugs discovered during testing Partially completed test cases and related resources
          Hide
          Dag H. Wanvik added a comment -

          Thank, Hiranya!
          Hopefully we can by joined efforts make those missing parts of the test code ready in time for 10.6. I'll have a look asap.

          Show
          Dag H. Wanvik added a comment - Thank, Hiranya! Hopefully we can by joined efforts make those missing parts of the test code ready in time for 10.6. I'll have a look asap.
          Hide
          Rick Hillegas added a comment -

          Linking this issue to DERBY-4485. Revision 892627 added dblook support for UDTs and for the USAGE privileges on them. We will need to wire UDTs and their USAGE privileges into the work being done on this issue. This is my understanding of the dependency ordering:

          UDTs < Routines < Tables

          Show
          Rick Hillegas added a comment - Linking this issue to DERBY-4485 . Revision 892627 added dblook support for UDTs and for the USAGE privileges on them. We will need to wire UDTs and their USAGE privileges into the work being done on this issue. This is my understanding of the dependency ordering: UDTs < Routines < Tables
          Hide
          Rick Hillegas added a comment - - edited

          Also linking to DERBY-4487. We will be adding dblook support for SEQUENCEs and the USAGE privileges on them. We will need to wire those objects into the work being done on this issue. I believe this is the dependency ordering:

          Sequences < Views,Triggers

          Show
          Rick Hillegas added a comment - - edited Also linking to DERBY-4487 . We will be adding dblook support for SEQUENCEs and the USAGE privileges on them. We will need to wire those objects into the work being done on this issue. I believe this is the dependency ordering: Sequences < Views,Triggers
          Hide
          Dag H. Wanvik added a comment -

          Unsetting the "patch available" flag , since more works needs to be done on this issue.

          Show
          Dag H. Wanvik added a comment - Unsetting the "patch available" flag , since more works needs to be done on this issue.

            People

            • Assignee:
              Hiranya Jayathilaka
              Reporter:
              Hiranya Jayathilaka
            • Votes:
              1 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development