Change syntax for Oracle multi-row insert SQL statement

[abepolk]

A fix for a bug involving SQL inserts in Oracle. This is not final yet.
See #5812 (comment)

• Change in CHANGELOG.md described (if applicable)
• Tests created for changes (if applicable)
• Manually tested changed features in running JabRef (always required)
• Screenshots added in PR description (for bigger UI changes)
• Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

[abepolk]

Is it possible to run the Oracle tests on GitHub? The fix ended up being really simple and it probably only needs to be tested once or twice. This might be easier than installing Docker.

[tobiasdiez]

They oracle tests are now invoked correctly.

[abepolk]

Thanks! Is it normal for ccrypt to throw an error here in the Oracle tests?

[tobiasdiez]

I don't think so.

@koppor ?

[koppor]

• the docker container needs the Oracle RPM

• Oracles requires agreement to the terms of use (not using it for terr.r, e.g.)

• I wanted to enable the tests on our CI solution

• Therefore, I stored the RPM encrypted

• Without a secret, a decryption error is thrown.

  ccrypt: files/oracle-database-xe-18c-1.0-1.x86_64.rpm.cpt: key does not match -- unchanged
  

• The secret to the encryption is stored in our GitHub secrets

• GitHub secrets are not shared to external pull requests:

  With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

Therefore, @NorwayMaple gets the error and me not.

Solution: Someone of the core developers has to fetch the branch and push it into our main repo. Then wait for the check and then delete the branch again (to avoid confusion; especially because of a missing pull request (to THAT branch!) and the possible outdated branch)

I did it for https://github.com/JabRef/jabref/commit/f98c9b604f2cfa77376369770346e5e8814b2464/checks?check_suite_id=407206367 (needed to patch our oracle workflow -> ec618d7)

[koppor]

Did not succeed. E.g.,

  Test testGetSharedIDVersionMapping() FAILED

  org.opentest4j.AssertionFailedError: expected: <{1=1, 2=2}> but was: <{1=1, 2=1}>
      at org.jabref.logic.shared.DBMSProcessorTest.testGetSharedIDVersionMapping(DBMSProcessorTest.java:312)

[abepolk]

Okay. I could still look into using Docker to work with Oracle locally - Docker is a useful skill in the job market anyway. I was just seeing if this might work as a quick fix.

[abepolk]

I got Oracle to work in a docker container. To return to the JDBC, I am reading on Stack Overflow that it is actually better use the "batch" functionality of PreparedStatement rather than make very long single SQL queries. See, for example: https://stackoverflow.com/questions/4901997/oracle-and-jdbc-performance-insert-all-vs-preparedstatement-addbatch/4909200

Does that sound right? Should I switch to PreparedStatement batches?

[abepolk]

I am currently getting an error when I run databaseTest with the Oracle DB container running saying the following:

  java.sql.SQLSyntaxErrorException: ORA-00955: name is already used by an existing object
      at org.jabref.logic.shared.DBMSProcessorTest.setup(DBMSProcessorTest.java:48)
  Caused by: Error : 955, Position : 13, Sql = CREATE TABLE "ENTRY" ("SHARED_ID" NUMBER NOT NULL, "TYPE" VARCHAR2(255) NULL, "VERSION" NUMBER DEFAULT 1, CONSTRAINT "ENTRY_PK" PRIMARY KEY ("SHARED_ID")), OriginalSql = CREATE TABLE "ENTRY" ("SHARED_ID" NUMBER NOT NULL, "TYPE" VARCHAR2(255) NULL, "VERSION" NUMBER DEFAULT 1, CONSTRAINT "ENTRY_PK" PRIMARY KEY ("SHARED_ID")), Error Msg = ORA-00955: name is already used by an existing object
      at org.jabref.logic.shared.DBMSProcessorTest.setup(DBMSProcessorTest.java:48)

It seems that the tables already exist at the beginning of each test. At least in DBMSProcessorTest, they shouldn't because of TestManager.clearTables(this.dbmsConnection);
Any ideas what is going on? The settings are basically the same as those on the GitHub test, except that I removed .setPort(32188) in TestConnector in order to get it to connect to my container properly.

[abepolk]

I've been working on the issue with dropping tables, and it looks like the PL/SQL statement in TestManager is acting up locally, but not on GitHub CI:

BEGIN
EXECUTE IMMEDIATE 'DROP TABLE "FIELD"';
EXECUTE IMMEDIATE 'DROP TABLE "ENTRY"';
EXECUTE IMMEDIATE 'DROP TABLE "METADATA"';
EXECUTE IMMEDIATE 'DROP SEQUENCE "ENTRY_SEQ"';
EXCEPTION
WHEN OTHERS THEN
IF SQLCODE != -942 THEN
RAISE;
END IF;
END;

I looked at the Oracle SQL docs, and it looks like this is a statement that drops the three tables if they exist, and if not (SQLCODE is 942) then it ignores the exception. It raises all other exceptions. When I run this block through JabRef and the JDBC it uses, nothing happens, and I get an error later in the tests when it tries to create the "ENTRY" table saying it already exists. Perhaps a 942 exception in dropping "FIELD" is short-circuiting the other table drops? But that still doesn't explain why it works on the GitHub CI.

[tobiasdiez]

@koppor ideas?

[abepolk]

I figured it out. It works out that because the table drops were all in the same PL/SQL block, when one threw an exception, the other table drops were ignored. I solved this by putting each drop in a separate PL/SQL block so that each one is dropped if it exists, even if previous one threw an exception for not existing. See commit bbc81ab. I also fixed SQL typos in OracleProcessor. The tests now pass on the Oracle DB in my local container. @koppor Can you push this to master and see if the Oracle tests pass now?

[koppor]

I enabled running the Oracle tests always and pushed to fix_fields_sql in our repo. See https://github.com/JabRef/jabref/actions/runs/33163398 for the tests.

In case there are updates, we can merge your updates to fix_fields_sql in this repository.

As soon as your PR is merged into master, we will delete fix_fields_sql in our repo (as everything is in master)

[koppor]

Since this touches Oracle only, I'll go ahead for a merge - to keep things going.