Feature/fix modification time bug #946

Merged
merged 4 commits into from Mar 27, 2016

Conversation

Projects
None yet
3 participants
@olvidalo
Contributor

olvidalo commented Mar 19, 2016

When trying to overwrite an XML resource with an invalid XML resource, the modification time of the resource would still be updated even if the resource has not been changed. This is due to the method manageDocumentInformation in validatateXMLResource being called before the document is validated (in Collection.java).

I have moved the modification time update out of manageDocumentInformation into a separate method which will only be called after the document is validated. There's also a test that will fail without the changes in Collection.java and verifies that modification times will still be updated as expected for valid resources.

This fixes the underlying issue for bug #811.

+ */
+
+ private void updateModificationTime(final DocumentImpl document) {
+ DocumentMetadata metadata = new DocumentMetadata();

This comment has been minimized.

@dizzzz

dizzzz Mar 20, 2016

Member

Why note write like
DocumentMetadata metadata = document.getMetadata();
?

@dizzzz

dizzzz Mar 20, 2016

Member

Why note write like
DocumentMetadata metadata = document.getMetadata();
?

This comment has been minimized.

@adamretter

adamretter Mar 20, 2016

Member

Also can you make use of final unless you variable need to be mutable for some reason please?

@adamretter

adamretter Mar 20, 2016

Member

Also can you make use of final unless you variable need to be mutable for some reason please?

@dizzzz

This comment has been minimized.

Show comment
Hide comment
@dizzzz

dizzzz Mar 20, 2016

Member

Change looks good to me, was a bit puzzled (in the diff tool) about the binary document handling

Member

dizzzz commented Mar 20, 2016

Change looks good to me, was a bit puzzled (in the diff tool) about the binary document handling

+
+
+ @BeforeClass
+ public static void ensureCleanDatabase() throws IOException {

This comment has been minimized.

@adamretter

adamretter Mar 20, 2016

Member

It seems to me that this would be useful for other tests too.
Could you move the logic of this method into org.exist.TestUtils?

@adamretter

adamretter Mar 20, 2016

Member

It seems to me that this would be useful for other tests too.
Could you move the logic of this method into org.exist.TestUtils?

+ assertEquals(modificationTimeBefore, modificationTimeAfter);
+ }
+
+ protected BrokerPool startDB() throws DatabaseConfigurationException, EXistException {

This comment has been minimized.

@adamretter

adamretter Mar 20, 2016

Member

Could this not have an @Before annotation and be called once per-test, or better yet if it's possible then @BeforeClass so that the database only need be started once for the suite of tests (you would also need to change your @After to @AfterClass.

@adamretter

adamretter Mar 20, 2016

Member

Could this not have an @Before annotation and be called once per-test, or better yet if it's possible then @BeforeClass so that the database only need be started once for the suite of tests (you would also need to change your @After to @AfterClass.

@adamretter

This comment has been minimized.

Show comment
Hide comment
@adamretter

adamretter Mar 20, 2016

Member

@olvidalo Welcome and thank you for your PR :-) Just a couple of small comments inline, otherwise it looks good to me.

Member

adamretter commented Mar 20, 2016

@olvidalo Welcome and thank you for your PR :-) Just a couple of small comments inline, otherwise it looks good to me.

@olvidalo

This comment has been minimized.

Show comment
Hide comment
@olvidalo

olvidalo Mar 21, 2016

Contributor

Hey thanks for the welcome and all your suggestions.

@adamretter I tried having the database start only once in a @beforeClass method and stopping it in @afterClassbut while this works fine when I run the ModificationTimeTest by itself, I ran into problems when running the test at the end of ant test-local. Sometimes I would get a random NullPointerException or failed assertions, each time somewhere else. Now what do you think, should I commit these changes so that maybe someone more knowledgeable about eXist's internal workings than me could have a look at it or should I just go with the @Before approach, starting the database before each test?

Contributor

olvidalo commented Mar 21, 2016

Hey thanks for the welcome and all your suggestions.

@adamretter I tried having the database start only once in a @beforeClass method and stopping it in @afterClassbut while this works fine when I run the ModificationTimeTest by itself, I ran into problems when running the test at the end of ant test-local. Sometimes I would get a random NullPointerException or failed assertions, each time somewhere else. Now what do you think, should I commit these changes so that maybe someone more knowledgeable about eXist's internal workings than me could have a look at it or should I just go with the @Before approach, starting the database before each test?

@dizzzz

This comment has been minimized.

Show comment
Hide comment
Member

dizzzz commented Mar 22, 2016

@adamretter adamretter merged commit fa3dc44 into eXist-db:develop Mar 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment