icatproject / icat.server

The ICAT server offering both SOAP and "RESTlike" interfaces to a metadata catalog.
Other
1 stars 5 forks source link

Correct number of results returned #275

Closed kevinphippsstfc closed 2 years ago

kevinphippsstfc commented 2 years ago

Closes #273

RKrahl commented 2 years ago

I'm a little puzzled. I don't get any failing tests from the current master branch version. But when I try this one, I get:

$ mvn install
[...]
[INFO] --- maven-failsafe-plugin:2.18.1:integration-test (default) @ icat.server ---
[INFO] Failsafe report directory: /home/abuild/src/icat.server/target/failsafe-reports

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
[...]
Tests run: 27, Failures: 1, Errors: 0, Skipped: 2, Time elapsed: 37.631 sec <<< FAILURE! - in org.icatproject.integration.TestRS
testSearch(org.icatproject.integration.TestRS)  Time elapsed: 0.771 sec  <<< FAILURE!
java.lang.AssertionError: expected:<5> but was:<4>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.icatproject.integration.TestRS.search(TestRS.java:1033)
        at org.icatproject.integration.TestRS.testSearch(TestRS.java:690)

I didn't analyze this test to check what ought to be the correct result, though.

kevinphippsstfc commented 2 years ago

Now I'm confused by this too! I fixed it because the test was failing for me and when I investigated the code there was the comment (from Steve 6 years ago!) "TODO this is wrong - there should be 4 false and 1 true" which appeared to be true for me. I did wonder how the tests have presumably been passing since then every time a release is made.

So now I am wondering whether it is something to do with the setup of the ICAT that is used for running the tests, for example, the Java version or the MySQL/MariaDB version. I am using OpenJDK 1.8.0_282 on Ubuntu 18.04 with MariaDB 10.1.47.

If you look at the Datasets that are being imported for the test in the icat.port file:

Dataset (investigation(facility(name:0), name:1, visitId:2) , name:3, type(facility(name:0), name:4), complete:5, startDate:6, endDate:7 sample(investigation(facility(name:0), name:1, visitId:2), name:8), description:9)
"Test port facility", "expt1", "one", "ds1", "calibration", true,  2014-05-16T16:58:26.12+12:30, 2014-05-16T16:58:26.12+12:30, "Koh-I-Noor", "alpha"
"Test port facility", "expt1", "one", "ds2", "calibration", null,  2014-05-16T06:05:26.12Z, 2014-05-16T06:07:26.12+12:30,"Ford\t\"Anglia\"", "beta"
"Test port facility", "expt1", "one", "ds3", "calibration", False, 2014-05-16T06:09:26.12+01:00, 2014-05-16T06:15:26.12+01:00, null, "gamma"
"Test port facility", "expt1", "two", "ds3", "calibration", False, 2014-05-16T06:20:26.12, 2014-05-16T06:21:26.12, null, "delta"
"Test port facility", "expt1", "two", "ds4", "calibration", False, 2014-05-16T06:31:26.12, 2014-05-16T06:32:26.12, null, "epsilon"

then there are 5 Datasets but I am wondering whether the Dataset 'complete' field that is set to null maybe gets included by some ICAT configurations and not by others. Any thoughts?

Another thing I just noticed is that the test then goes on to count the number of "trues" and "falses" of which there should really be 1 true and 3 false (the fifth one being null), but it actually counts the number that are not true and counts 4. But I guess this is a minor detail compared to the fact that we are getting different numbers of results returned.

RKrahl commented 2 years ago

As I already wrote, I didn't analyze the test by now.

But the complete thing you show in the data is bizarre: complete has a not null constraint. It shouldn't be possible in the first place to create any dataset having complete set to null.

Another thing to consider when analyzing the test are access permissions. Does the user "notroot" has read permission to all datasets? If not, you would get less results on the search.

kevinphippsstfc commented 2 years ago

Thanks for pointing out the not null constraint on complete - that is indeed bizarre!

Regarding the permissions for "notroot" - this user has the permissions to see all Datasets.

So I checked what Datasets are put into my ICAT when I run the tests and all 5 of the Datasets in the icat.port file appear in the database. The second Dataset ds2 with the null COMPLETE field actually appears in the database with the COMPLETE field set to 0 (False) so my results are what I would expect: 1 true and 4 false.

If I check the DATASET table in my database:

MariaDB [icatdb]> describe DATASET;
+------------------+--------------+------+-----+---------+----------------+
| Field            | Type         | Null | Key | Default | Extra          |
+------------------+--------------+------+-----+---------+----------------+
| ID               | bigint(20)   | NO   | PRI | NULL    | auto_increment |
| COMPLETE         | tinyint(1)   | NO   |     | 0       |                |

then the default value for the COMPLETE field is 0, so I guess the database is accepting the null value and just silently inserting the default value of 0 when the database is populated.

I found this about constraints in MariaDB:

https://mariadb.com/kb/en/constraint/#check-constraints

which says that "From MariaDB 10.2.1, constraints are enforced. Before MariaDB 10.2.1 constraint expressions were accepted in the syntax but ignored."

I am using version 10.1.47 on Ubuntu 18.04, so the constraints on my database are possibly not being used.

What version of mysql/mariaDB are you using @RKrahl ?

kevinphippsstfc commented 2 years ago

@RKrahl if you could let me know what version of the JDBC driver jar you are using that might help as well.

RKrahl commented 2 years ago

I use for the test:

mariadb and java are the default packages that come with openSUSE Leap 15.3. mysql-connector-java is a downgrade from the older openSUSE Leap 42.3, because the one that comes with 15.3 does not work with Java 8 any more.

kevinphippsstfc commented 2 years ago

Thanks Rolf. One other thing I wanted to check was what storage engine you are using. InnoDB seems to be the default in mariadb since version 10.2 and is the one we use when we set up ICAT using icat.ansible

RKrahl commented 2 years ago

Yes, I was also using InnoDB in the test. Actually, I didn't actively select it, but just stick to the default InnoDB.

RKrahl commented 2 years ago

The closer you look at these things, the weirder they seem to get. I tried to connect to ICAT with python-icat in paralell to the tests. At the spot of this change I get:

>>> client.search("SELECT ds.complete FROM Dataset ds")
[False, False, False, False]
>>> dss = client.search("SELECT ds FROM Dataset ds")
>>> len(dss)
5
>>> dss
[(dataset){
   createId = "db/notroot"
   createTime = 2022-05-19 09:35:05+00:00
   id = 597
   modId = "db/notroot"
   modTime = 2022-05-19 09:35:05+00:00
   complete = True
   description = "alpha"
   endDate = 2014-05-16 04:28:26+00:00
   name = "ds1"
   startDate = 2014-05-16 04:28:26+00:00
 }, (dataset){
   createId = "db/notroot"
   createTime = 2022-05-19 09:35:05+00:00
   id = 598
   modId = "db/notroot"
   modTime = 2022-05-19 09:35:05+00:00
   complete = False
   description = "beta"
   endDate = 2014-05-15 17:37:26+00:00
   name = "ds2"
   startDate = 2014-05-16 06:05:26+00:00
 }, (dataset){
   createId = "db/notroot"
   createTime = 2022-05-19 09:35:05+00:00
   id = 599
   modId = "db/notroot"
   modTime = 2022-05-19 09:35:05+00:00
   complete = False
   description = "gamma"
   endDate = 2014-05-16 05:15:26+00:00
   name = "ds3"
   startDate = 2014-05-16 05:09:26+00:00
 }, (dataset){
   createId = "db/notroot"
   createTime = 2022-05-19 09:35:05+00:00
   id = 600
   modId = "db/notroot"
   modTime = 2022-05-19 09:35:05+00:00
   complete = False
   description = "delta"
   endDate = 2014-05-16 06:21:26+00:00
   name = "ds3"
   startDate = 2014-05-16 06:20:26+00:00
 }, (dataset){
   createId = "db/notroot"
   createTime = 2022-05-19 09:35:05+00:00
   id = 601
   modId = "db/notroot"
   modTime = 2022-05-19 09:35:05+00:00
   complete = False
   description = "epsilon"
   endDate = 2014-05-16 06:32:26+00:00
   name = "ds4"
   startDate = 2014-05-16 06:31:26+00:00
 }]
>>> [ds.complete for ds in dss]
[True, False, False, False, False]

The first search, searching for the attributes only, is what the test tries. It apparently yields a wrong result. Looks like we have a bug in icat.server.

(Note that python-icat uses the SOAP interface, while this test uses the REST interface, but the results I get from python-icat are consistent with what I see in the test.)

kevinphippsstfc commented 2 years ago

The strange thing is that this only seems to be happening on your test setup.

On my test setup, using either mariadb or oracle as the database backend, and also on the github actions setup (using icat.ansible), the test is returning 5 results of which 1 is true and 4 are false.

That's why I'm struggling to investigate this one. I can't reproduce the case of only getting 4 results returned.

kevinphippsstfc commented 2 years ago

Just adding a note that Louise thinks this test must have been passing using the Travis CI setup so there may be something that has in common with Rolf's setup that is causing the different result.

RKrahl commented 2 years ago

I believe I got a significant step closer to narrow the case down: I did all my building and testing of icat.server with Payara 4 before. Now, I upgraded also my build environment to Payara 5 and I get the same results as you. E.g. the test as of master fails and the corrected version from the from the 273-fix-failing-testrs branch succeed for me.