Re: [stellation-res] Database Package Audit.

["Mark C. Chu-Carroll" <mcc@xxxxxxxxxxxxxxxxxxxxx>]

Please use "stellation-res@xxxxxxxxxxxxxxx" instead of
"stellation-res@xxxxxxxxxxx". It messes up various people's email
filters to use the eclipse address rather than the dev.eclipse address.

Thanks for the thorough pass over the code. I'll be working on
addressing your comments for my next checkin.

Since it'll probably take me a few days to get the time to
take care of all of this, I'm going to try to address a few of the
confusing problems that you found here, so that anyone who wants to
try working with the code will understand what's going on.

First: the options list.


A location string is formed from a prefix, which identifies the kind
of location, and a series of colon-separated options. The values of
those options varies depending on the database.

For postgres, a location string is:
	postgres:dbname

So the options list 

For DB2, it's more complicated. A db2 location is:
	db2:dbname:dbuser:dbpasswd

So the options list will be a database name, the name of the user used
to connect to the database, and the password used to connect to the
database.

Second: Only postgres database is current enabled.

That's deliberate. This is a first cut at the system, which I wanted to
give people an opportunity to look at ASAP. Since I only tested the
postgres version, I decided to leave it with only the known working
version enabled. 

You are entirely correct that it should not just be using a literal
string that way. I've been debating whether the database list should be
hardcoded using static strings and an explicit conditional, or whether
it should be based on a configuration file that specifies the mapping
of location prefix to access point/database, and uses introspection
to get the database instances. I finally decided on the more static
route, but didn't quite do it as completely as I should have.

Third: about transactions. The intention is that everything always be in
an explicitly committed transaction. You are absolutely correct that
if that's the way we want it to work, it should trigger an error
when you don't work within a transaction. I will fix that before the
next checkin. 

Fourth: I did not realize that the default value for long string would
cause on error on MySQL. I don't have MySQL manuals handy at the moment:
what's the problem with it? Is it that the size is too long? Or is
it something more subtle? 


The rest doesn't need any comment: you've found some real, serious
problems, and we will fix them, ASAP.

	-Mark


On Sat, 2002-08-31 at 12:56, Jonathan Gossage wrote:
> Attached is a text file that contains a file by file discussion of all the
> points that I noticed in reviewing the database package classes. Enjoy :-)
> 
> Regards
> 
> Jonathan Gossage
> 
> Personal Email
> jgossage@xxxxxxxx
> 
> Business Email
> jonathan@xxxxxxxxxxxxxx
> 
> ----
> 

> This document  contains a file by file audit of the new database package.
> 
> 
> org.eclipse.stellation.repos.database.AbstractDatabase
> 
> Javadoc for connectToDatabase
> 
> The param "options" is a good example of a method argument whose content cannot be determined from the name and type. In this particular case we have to follow the reference chain back through AbstractBBAccessPoint, where we discover that it is an argument to the constructor named "location". Moving on backward we find in the method workspace.Configure.database that this argument comes from an  option named "location" in a set of options. We also find here that these options are stored in a workspace.Context object which is passed in when the workspace.Configure object is constructed. Again, moving back we can discover that the workspace.Configure object is constructed in the workspace.Svc.command method which receives the context as a parameter. Moving back yet once more we find that workspace.Svc.command is invoked from workspace.Svc.run where we finally discover where the workspace.Context object is created. We also discover that the options are embedded in the field "programArguments which is passed to the workspace.Svc.run method. Moving back one final step we discover that the options came from the arguments passed to main on the command line.
> 
> Having got here we at least know where this location option came from but we still do not know what it looks like. It appears we need to go to external documentation to discover this information. The point of this tedious litany is that a substantial amount of work is needed to discover the rules governing the content of the "location" argument. This work can be greatly reduced by following one simple convention.
> 
> Use @see with an external hyperlink to link to the section in the document that describes the contents of the argument or use an internal link if the contents of the argument are defined internally within the Javadoc..
> 
> I promise I will not be so long winded in future but following this convention will save substantial amounts of time, particularly for newcomers to the code base. The only problem could come when major documentation restructuring breaks the link but a Javadoc audit as part of each major release should deal with this problem.
> 
> 
> org.eclipse.stellation.repos.database.AbstractDBAccessPoint
> 
> 1. The Javadoc for the constructor describes the previous implementation of the constructor. The argument list has changed. Again need a hyperlink for the argument "location".
> 
> 2. The constructor uses a hard coded string to determine the database type. These strings are scattered throughout the code base in several modules. Should the database names not be stored in String constants so they can be used consistently wherever needed.
> 
> 3. Also some thought needs to be given to whether knowledge of the specific database in use should be so widely scattered. Ideally all other code should need is an access point. At the moment there is database sensitive code in the following non-database modules:
> 
> 	org.eclipse.stellation.repos.LocalHandle
> 	org.eclipse.stellation.workspace.Location
> 	org.eclipse.stellation.scm.model.ScmAccessSpec
> 	org.eclipse.stellation.scm.model.ScmEnvirons
> 	org.eclipse.stellation.repos.artifact.TextArtifact
> 
> 4. The Javadoc for the createRepository method needs to be updated. The parameter list has changed. Also there should be a hyperlink to a documentation section that discusses the contents of the JDOM document. It is very difficult to keep documentation in sync. if it is duplicated. Since most of the tables are created here, a hyperlink to the document that describes the database tables should be included here.
> 
> 5. The "createRepository" method contains a startTransaction call but no endTransaction call. Unless special circumstances hold, it is much safer from a code maintenance viewpoint to have both the start and end of a transaction in the same method. When this is not possible, it should be clearly documented at the point where the startTransaction call is made where the endTransaction call will be made.
> 
> 6. Do you not require Javadoc for getter methods? There is none for "getRepositoryTag".
> 
> 7. "getAuthenticator" would benefit from a hyperlink to the point where authentication methods are discussed in the documentation.
> 
> 8. "instantiateAgents" is logging exceptions to stderr. Should it not be using Log4J?
> 
> 9. "getDatabase" needs a Javadoc entry.
> 
> 10. "supportsDBType" needs a Javadoc entry.
> 
> 
> org.eclipse.stellation.repos.database.BasicDatabase
> 
> 1. The implementation of "getLongStringType" will generate incorrect code for some database systems unless overridden. For example it could generate VARCHAR(4000) which is illegal in MySql. My feeling is that BasicDatabase should only provide default implementations for types that can be guaranteed to be usable, even if not optimal, on all database systems Accordingly, I would provide no implementation in BasicDatabase for "getStringType" and "getLongStringType".
> 
> 2. Javadoc entries should be supplied for protected methods since these are used in derived classesand are of interest to developers. The following methods need Javadoc entries:
> 	renderField
> 	renderPrimaryKey
> 	renderForeignKey
> 	getCreateTableHeader
> 	getCreateTableTrailer
> 
> 3. You may want to consider setting auto commit to true in endTransaction. This would provide a safety net that would ensure the integrity of any SQL statements that were inadvertantly not included in a transaction. Alternatively if you want a policy that forces all SQL statements to be run in transactions, you need to add a mechanism to check for this and throw an exception if the contract is violated.
> 
> 4. So that I understand, why do you use protected fields rather than providing protected accessor functions for derived classes. In general it is best to encapsulate data as fully as possible.
> 
> 
> org.eclipse.stellation.repos.database.DB2Database
> 
> 1. The error text in the exception messages refers to Postgres rather than DB2.
> 
> 
> org.eclipse.stellation.repos.database.Table
> 
> 1. All methods need Javadoc.
> 
> 
> org.eclipse.stellation.repos.database.TemplateExpander
> 
> 
-- 
Mark Craig Chu-Carroll,  IBM T.J. Watson Research Center  
*** The Stellation project: Advanced SCM for Collaboration
***		http://www.eclipse.org/stellation
*** Work Email: mcc@xxxxxxxxxxxxxx  ------- Personal Email:
markcc@xxxxxxxxxxx