-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance the JDBC Driver Manager to validate all driver supported properies (JDBC-driver, #1825) #1827
Enhance the JDBC Driver Manager to validate all driver supported properies (JDBC-driver, #1825) #1827
Conversation
...e.birt.report.data.oda.jdbc/src/org/eclipse/birt/report/data/oda/jdbc/JDBCDriverManager.java
Show resolved
Hide resolved
...e.birt.report.data.oda.jdbc/src/org/eclipse/birt/report/data/oda/jdbc/JDBCDriverManager.java
Outdated
Show resolved
Hide resolved
FYI, the build failures might be related to the new version of derby that I just made available in Orbit. I was just about to test a local build. |
4205cac
to
b344a48
Compare
...e.birt.report.data.oda.jdbc/src/org/eclipse/birt/report/data/oda/jdbc/JDBCDriverManager.java
Outdated
Show resolved
Hide resolved
FYI, tests are failing with the latest derby:
I'm not an expert in how derby should actually work. 😱 |
FYI, the tests are always using the same temp folder name and when I deleted that from my temp, the tests succeeded. So hopefully there are no problems with derby. But I need to verify that before I can spend time on this PR.... |
Confirmed, I cannot see any difference too. |
b344a48
to
a40a72e
Compare
I think my local failures are not actually related to the derby changes. They're just like the problems I had when I was trying get rid of the base64 encoding stuff. In any case, I have another workspace where I could look at your PR. I think something like this would do the same thing but is a bit simpler:
The keySet() supports remove and we use predicate to specific which keys to remove... |
a40a72e
to
11ccfb4
Compare
Looks very nice, I replaced my version through you suggestion. |
11ccfb4
to
d35f522
Compare
These test failures really are test failures caused by the changes in this PR. For example, I see |
This works better:
A few of the tests fail randomly when I launch them in the IDE, but hopefully the above consistently passes the tests. Hopefully there are not other properties that might be important that might get removed unexpectedly with this change. |
Thanks! I will add the special sequence and then we will see with crossed fingures. |
properties and remove the unsupported from the connection properties
d35f522
to
0add57f
Compare
I fetched your changes to my fork and the project recreation is currently running on my side locally. |
@merks But the result of my local test of the "ODA Data" is very red with multiple findings in compare to our 4 failures. The core issue message is like originally on your screen where you start with the derby investigation. |
Yes, that got me too in the beginning and required some debugging. Delete this from the temp folder. On Windows
This folder from your test failure: Better that the tests cleaned it properly. 😢 I'm going out now so help will be delayed until after dinner. At least his PR's test passed. 🥳 |
Yes, the folder clean up help with the listed errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A work of art. 🖼️
Enhance the JDBC Driver Manager to validate all driver supported properties and remove the unsupported from the connection properties, e.g. not all drivers support Bidi-properties according to DuckDB (JDBC-driver, #1825)