[ http://issues.apache.org/jira/browse/DERBY-1547?page=comments#action_12435003 ] John H. Embretsen commented on DERBY-1547: ------------------------------------------
I looked a bit closer at some of the substitution patterns in the latest patch, 'DERBY-1547-sed-v1.diff ', and my conclusion is that they most likely will match _most_ of the expected strings (there is one issue I think will cause problems, see below), but that they will also match strings that are potentially very different from an SVN revision number. I don't think this is a major issue, but having as restrictive regular expressions as possible will reduce the chance of unintentionally masking regressions in this area. Perhaps there is not much chance of regressions in version strings like this, but I thought I'd report my findings anyway. I have mostly examined patterns related to jdbcapi/metadata.java, but patterns for other tests look very similar. I've based most of my RegEx interpretations on explanations provided on this website: http://www.regular-expressions.info/quickstart.html Issues that should be fixed before committing: ==================================== The new pattern that I think needs to be changed is: ....[0-9][0-9][0-9][0-9][0-9][0-9][0-9]*[A-Z]. This will match the SVN rev 446536M, but not 446536. As far as I know, the M means that a modified version (i.e. uncommitted changes) is being used. Tests of unmodified versions of Derby will not include the M in the SVN revision number, and the test will fail. Please correct me if I am wrong. For this reason, I think the above pattern should be changed to the following: [(][1-9][0-9]{5,}[A-Z]?[)] This also incorporates other changes, which are described further below. Perhaps even [(][1-9][0-9]{5,}M?[)] will match in all cases, but I'm not entirely sure (SVN gurus will have to confirm or deny). The "{5,}" means "5 or more repetitions of the preceding token". The patch includes this pattern in the following files: jdbcapi/dbMetaDataJdbc30_sed.properties jdbcapi/odbc_metadata_sed.properties jdbcapi/metadata_sed.properties A similar pattern, only with an extra "dot" at the end, is included in derbynet/NSinSameJVM_sed.properties derbynet/testProperties_sed.properties These derbynet patterns should be changed the same way as the jdbcapi patterns, provided my reasoning above is correct. Also, the extra "dot" does not seems to make much of a difference. Why is it there? I think if the trailing space in the replacement string ("-(EXPECTED SUBVERSION INFO) ") is removed, then the extra "dot" can be removed too. Minor issues (the patch may be committed without fixing them): =========================================== * Some of the new replacement strings modify more of the output than just the SVN revision number. I think only the revision number should be replaced. For example, the output version 10.3 (10.3.0.0 alpha - (446536M)) will end up as version 10.3 (10.3.0.0 alpha-(EXPECTED SUBVERSION INFO)) I think it would be better to have it like this: version 10.3 (10.3.0.0 alpha - (EXPECTED SUBVERSION INFO)) or perhaps like this: version 10.3 (10.3.0.0 alpha - (xxxxxxFILTERED-SVN-REVISIONxxxxx)) (the latter makes it more clear that something has been filtered out). The following pattern will do that, I believe: [(][1-9][0-9]{5,}[A-Z]?[)];(xxxxxxFILTERED-SVN-REVISIONxxxxx) Other comments: New patterns added by the patch: ------------------------------------------------------------ The new patterns are supposed to match subversion revision numbers. For example, the regEx ....[0-9][0-9][0-9][0-9][0-9][0-9][0-9]*[A-Z]. will match the revision number in the string version 10.3 (10.3.0.0 alpha - (446536M)) This is OK, but a bit too liberal. The "dot" (.) in a Java regEx matches almost any character, unless it is inside a character class, []. This, and the use of the * character, means that the regEx will also match the following string: version FAIL99999999999999999999999999AA Existing patterns (not changed by the patch): ------------------------------------------------------------------------------- It seems to me that the existing patterns for the tests touched by this patch are supposed to match JCC versions when running in the DerbyNet framework. For example, the regEx version [0-9][.][0-9]..[0-9][.][0-9][.][0-9][0-9]?[0-9]?.[A-Za-z ]*.$ will match the string version 2.4 (2.4.17) This is OK, but a bit too liberal. The "dot" (.) in a Java regEx matches almost any character, unless it is inside a character class, []. This, and the use of the * character, means that the regEx will also match the following string: version 0.0 v9.9.9-someInvalidVersionString Perhaps the patterns (i.e., the minor issues) should be changed, but I think it would be more useful to spend time converting such tests to JUnit tests instead of tweaking the sed patterns (I'm not saying I volunteer to do that at this point in time, though). If anyone disagrees, I can create a new Jira issue for improving these substitution patterns. > Add svn version number to DatabaseMetaData getDatabaseProductVersion and > getDriverVersion() to improve supportability > ----------------------------------------------------------------------------------------------------------------------- > > Key: DERBY-1547 > URL: http://issues.apache.org/jira/browse/DERBY-1547 > Project: Derby > Issue Type: Improvement > Components: JDBC > Affects Versions: 10.1.3.2 > Reporter: Kathey Marsden > Assigned To: V.Narayanan > Priority: Minor > Fix For: 10.2.1.0 > > Attachments: DERBY-1547-sed-v1.diff, DERBY-1547-sed-v1.stat, > DERBY-1547-sed.diff, DERBY-1547-sed.stat, DERBY-1547_v1.diff, > DERBY-1547_v1.stat, derbyall_fail.txt > > > getDatabaseProductVersion and getDriverVersion() report only the four digit > Derby version number and not the svn build number. It would be useful to > return the full version including the build number as sysinfo does: e.g. > "10.1.2.4 - (392472)", That way it will be clear from application logs that > collect this information exactly what revision level they are running if they > are using rolled up fixes on the maintenance branch between releases. > There may be risk in doing this however if applications are parsing the > version information, but hopefully they will use getDatabaseMajorVersion() , > getDatbaseMinorVersion, getDriverMajorVersion, and getDriverMinorVersion for > such proccessing. -- 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
