Issue 262890043: Fix issue with DQL DATETOSTRING function. (Closed)
|
Created:
9 years, 10 months ago by Srinivas Modified:
9 years, 10 months ago CC:
pjo, brett.michael.johnson_gmail.com Visibility:
Public. |
DescriptionFix issue with DQL DATETOSTRING function.
DATETOSTRING uses UTC rather than server time in Documentum 7.x. This leads to the same problem that using DATETOSTRING was intended to fix: we might get back a timestamp in a non-server timezone, and then hand it back to DQL using the DATE function where it is interpreted as server time. There is a new DATETOSTRING_LOCAL function in Documentum 7.0 that restores the 6.x behavior of DATETOSTRING. This function doesn't exist before 7.0, so we need to check the server version and use the appropriate function.
Code changed to detect server version and use DATETOSTRING_LOCAL for version 7 and above, and DATETOSTRONG for others.
Patch Set 1 #
Total comments: 21
Patch Set 2 : Changes as per review. #
Total comments: 8
Patch Set 3 : Changes as per review #Patch Set 4 : Added extra test to test that regex won't pass for version "7.5.0" #
Total comments: 11
Patch Set 5 : Final LGTM changes. #
MessagesTotal messages: 11
Please review the changes.
Sign in to reply to this message.
Brett found the description unclear. You might look at b/23226447 to see if that text is useful. John L https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: static String DATETOSTRING_FUNCTION; Hmm. This isn't a constant, so it shouldn't be all caps. I don't think it should be static, either, since it's set by init. Also, I think you should make it private and have a @VisibleForTesting getter, though we have directly used the checkpoint fields from the tests, so maybe we should stay consistent with that (questionable) design. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:351: dmSession.getServerVersion().startsWith("7.") ? "DATETOSTRING_LOCAL" This breaks when version 8 is released. It's better to hard-code the past, which is known. Could use !startsWith("6.") or !matches("^[456]\\."). I picked 4 as the first version with DFC, you could argue for 1-6 and 56, too, along with just 6 since that's the oldest we officially support (though technically I'm not sure we do anything that won't work on 5). https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:351: dmSession.getServerVersion().startsWith("7.") ? "DATETOSTRING_LOCAL" Since you have to break anways, you could just break before ? and join the 2nd and 3rd lines. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:613: .append(MessageFormat.format("{0}",DATETOSTRING_FUNCTION)) What's the MessageFormat for? https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: public void testDateToStringFunctionForServerVersion6() throws Exception { These names obscure the variation. Maybe just testDateToString_version6? https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:699: public void testDateToStringFunctionForServerVersion7() throws Exception { Add a test for 8 and 10. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") Less disruptive to git blame to reverse these and simply add a new line for DATETOSTRING_LOCAL.
Sign in to reply to this message.
https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/20 00:57:36, JohnL wrote: > Less disruptive to git blame to reverse these and simply add a new line for > DATETOSTRING_LOCAL. The problem with that is the first DATETOSTRING replacement will generate FORMATEDATETIME_LOCAL, which will lead to a less obvious replacement to correct that.
Sign in to reply to this message.
https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/20 05:18:44, Brett wrote: > On 2015/08/20 00:57:36, JohnL wrote: > > Less disruptive to git blame to reverse these and simply add a new line for > > DATETOSTRING_LOCAL. > > The problem with that is the first DATETOSTRING replacement > will generate FORMATEDATETIME_LOCAL, which will lead to a > less obvious replacement to correct that. Good catch (by both of you, probably). What about replaceAll("DATETOSTRING(_LOCAL)?", "FORMATDATETIME")
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: static String DATETOSTRING_FUNCTION; On 2015/08/20 00:57:36, JohnL wrote: > Hmm. This isn't a constant, so it shouldn't be all caps. I don't think it should > be static, either, since it's set by init. Also, I think you should make it > private and have a @VisibleForTesting getter, though we have directly used the > checkpoint fields from the tests, so maybe we should stay consistent with that > (questionable) design. Done. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:351: dmSession.getServerVersion().startsWith("7.") ? "DATETOSTRING_LOCAL" On 2015/08/20 00:57:36, JohnL wrote: > Since you have to break anways, you could just break before ? and join the 2nd > and 3rd lines. Done. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:351: dmSession.getServerVersion().startsWith("7.") ? "DATETOSTRING_LOCAL" On 2015/08/20 00:57:36, JohnL wrote: > This breaks when version 8 is released. It's better to hard-code the past, which > is known. Could use !startsWith("6.") or !matches("^[456]\\."). I picked 4 as > the first version with DFC, you could argue for 1-6 and 56, too, along with just > 6 since that's the oldest we officially support (though technically I'm not sure > we do anything that won't work on 5). Done. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:613: .append(MessageFormat.format("{0}",DATETOSTRING_FUNCTION)) On 2015/08/20 00:57:36, JohnL wrote: > What's the MessageFormat for? Oops, I was using these two lines in one statement, and later made it into two lines. And didn't remove the MessageFormat which is not needed anymore. https://codereview.appspot.com/262890043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:613: .append(MessageFormat.format("{0}",DATETOSTRING_FUNCTION)) On 2015/08/20 00:57:36, JohnL wrote: > What's the MessageFormat for? Done. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: public void testDateToStringFunctionForServerVersion6() throws Exception { On 2015/08/20 00:57:36, JohnL wrote: > These names obscure the variation. Maybe just testDateToString_version6? Done. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:699: public void testDateToStringFunctionForServerVersion7() throws Exception { On 2015/08/20 00:57:36, JohnL wrote: > Add a test for 8 and 10. Done. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/20 00:57:36, JohnL wrote: > Less disruptive to git blame to reverse these and simply add a new line for > DATETOSTRING_LOCAL. DATETOSTRING_LOCAL needs to be in the first line, as mentioned by Brett here. Thx Brett. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/20 00:57:36, JohnL wrote: > Less disruptive to git blame to reverse these and simply add a new line for > DATETOSTRING_LOCAL. Done.
Sign in to reply to this message.
https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/21 02:07:59, Srinivas wrote: > On 2015/08/20 00:57:36, JohnL wrote: > > Less disruptive to git blame to reverse these and simply add a new line for > > DATETOSTRING_LOCAL. > > Done. Did you miss my one-line regex or not like it? https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToString_fucntion; s/dateToString_fucntion/dateToStringFunction/ (note sp error, too) https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToString_fucntion = dmSession.getServerVersion().matches("([456]\\..*)") The parentheses in the regex don't add value. https://codereview.appspot.com/262890043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: public void testDateToString_version6() throws Exception { Maybe add a helper method, since these four methods differ only in the version string and expected function name?
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/21 02:29:07, JohnL wrote: > On 2015/08/21 02:07:59, Srinivas wrote: > > On 2015/08/20 00:57:36, JohnL wrote: > > > Less disruptive to git blame to reverse these and simply add a new line for > > > DATETOSTRING_LOCAL. > > > > Done. > > Did you miss my one-line regex or not like it? Missed it. Changed it. https://codereview.appspot.com/262890043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:771: .replace("DATETOSTRING", "FORMATDATETIME") On 2015/08/21 02:29:07, JohnL wrote: > On 2015/08/21 02:07:59, Srinivas wrote: > > On 2015/08/20 00:57:36, JohnL wrote: > > > Less disruptive to git blame to reverse these and simply add a new line for > > > DATETOSTRING_LOCAL. > > > > Done. > > Did you miss my one-line regex or not like it? Done. https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToString_fucntion; On 2015/08/21 02:29:07, JohnL wrote: > s/dateToString_fucntion/dateToStringFunction/ (note sp error, too) Thx, fixed it. https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToString_fucntion; On 2015/08/21 02:29:07, JohnL wrote: > s/dateToString_fucntion/dateToStringFunction/ (note sp error, too) Done. https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToString_fucntion = dmSession.getServerVersion().matches("([456]\\..*)") On 2015/08/21 02:29:07, JohnL wrote: > The parentheses in the regex don't add value. Yes, not needed. https://codereview.appspot.com/262890043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToString_fucntion = dmSession.getServerVersion().matches("([456]\\..*)") On 2015/08/21 02:29:07, JohnL wrote: > The parentheses in the regex don't add value. Done. https://codereview.appspot.com/262890043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: public void testDateToString_version6() throws Exception { On 2015/08/21 02:29:07, JohnL wrote: > Maybe add a helper method, since these four methods differ only in the version > string and expected function name? Done.
Sign in to reply to this message.
LGTM, with nits, but please wait for Marc to review. John L https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; Delete extra space before String. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; Move this lower down, below the statics. I'm not sensing much rational for the groups down there, but maybe a separate group between cabinetWhereCondition and the checkpoints? https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; Add javadoc, e.g., "The DQL function that returns the time in the server timezone." https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: throws Exception { s/Exception/DfException/ throughout the new methods. The trap door for Exception is to avoid "throws ThisException, ThatException, AndThatException". it's not meant to suggest using Exception in place of a more specific exception.
Sign in to reply to this message.
LGTM, with one additional suggestion. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToStringFunction = dmSession.getServerVersion().matches("[456]\\..*") Do you need to put in a carat (to mark the beginning of the string)? Otherwise, wouldn't this potentially match 7.4.2 ?
Sign in to reply to this message.
Final LGTM changes. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; On 2015/08/21 05:09:44, JohnL wrote: > Delete extra space before String. Done. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; On 2015/08/21 05:09:44, JohnL wrote: > Add javadoc, e.g., "The DQL function that returns the time in the server > timezone." Done. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: @VisibleForTesting String dateToStringFunction; On 2015/08/21 05:09:44, JohnL wrote: > Move this lower down, below the statics. I'm not sensing much rational for the > groups down there, but maybe a separate group between cabinetWhereCondition and > the checkpoints? Done. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToStringFunction = dmSession.getServerVersion().matches("[456]\\..*") On 2015/08/21 05:10:22, myk wrote: > Do you need to put in a carat (to mark the beginning of the string)? Otherwise, > wouldn't this potentially match 7.4.2 ? No, it doesn't match "7.4.2". Already added a test to test for this, testDateToString_version75() with "7.5.0000.0100 Win32.SQLServer" as version string. https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: dateToStringFunction = dmSession.getServerVersion().matches("[456]\\..*") On 2015/08/21 05:10:22, myk wrote: > Do you need to put in a carat (to mark the beginning of the string)? Otherwise, > wouldn't this potentially match 7.4.2 ? Done. https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: throws Exception { On 2015/08/21 05:09:44, JohnL wrote: > s/Exception/DfException/ throughout the new methods. > > The trap door for Exception is to avoid "throws ThisException, ThatException, > AndThatException". it's not meant to suggest using Exception in place of a more > specific exception. Done.
Sign in to reply to this message.
On 2015/08/21 06:03:24, Srinivas wrote: > Final LGTM changes. > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java > (right): > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: > @VisibleForTesting String dateToStringFunction; > On 2015/08/21 05:09:44, JohnL wrote: > > Delete extra space before String. > > Done. > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: > @VisibleForTesting String dateToStringFunction; > On 2015/08/21 05:09:44, JohnL wrote: > > Add javadoc, e.g., "The DQL function that returns the time in the server > > timezone." > > Done. > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:103: > @VisibleForTesting String dateToStringFunction; > On 2015/08/21 05:09:44, JohnL wrote: > > Move this lower down, below the statics. I'm not sensing much rational for the > > groups down there, but maybe a separate group between cabinetWhereCondition > and > > the checkpoints? > > Done. > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: > dateToStringFunction = dmSession.getServerVersion().matches("[456]\\..*") > On 2015/08/21 05:10:22, myk wrote: > > Do you need to put in a carat (to mark the beginning of the string)? > Otherwise, > > wouldn't this potentially match 7.4.2 ? > > No, it doesn't match "7.4.2". Already added a test to test for this, > testDateToString_version75() with "7.5.0000.0100 Win32.SQLServer" as version > string. > > https://codereview.appspot.com/262890043/diff/80001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:350: > dateToStringFunction = dmSession.getServerVersion().matches("[456]\\..*") > On 2015/08/21 05:10:22, myk wrote: > > Do you need to put in a carat (to mark the beginning of the string)? > Otherwise, > > wouldn't this potentially match 7.4.2 ? > > Done. > > https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... > File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java > (right): > > https://codereview.appspot.com/262890043/diff/80001/test/com/google/enterpris... > test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:689: > throws Exception { > On 2015/08/21 05:09:44, JohnL wrote: > > s/Exception/DfException/ throughout the new methods. > > > > The trap door for Exception is to avoid "throws ThisException, ThatException, > > AndThatException". it's not meant to suggest using Exception in place of a > more > > specific exception. > > Done. Committed Documentum repository https://github.com/googlegsa/documentum/commit/e8173a869341d0e12d6c1bf67b0566...
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
