Remove use of Properties for URL query parsing, fixes #6403

[EliasStihl]

Fixes #6403

Changes proposed in this pull request:

• Deprecate all methods in ParsingUtilities that return java.util.Properties.
• Introduce a new method, static public Map<String, String> parseParameters(HttpServletRequest request), as a replacement for parseUrlParameters. This new method utilizes the request.getParameterMap() functionality for its implementation.
• Modify all uses of parseUrlParameters to use the new method.
• Adjust all necessary code to accommodate the new return type (Map<String, String>).
• Deprecate all public methods ImportingUtilities that use java.util.Properties and make new versions of them using Map<String, String> instead.

[EliasStihl]

When I run the tests locally with ./refine test, all tests pass, any idea why the tests fail on the CI server @tfmorris ?

[tfmorris]

The tests pass locally for me as well, but a re-run of the CI failed as well, so it's not just a flaky test. Let me investigate...

[tfmorris]

Three of the Database tests fail when run in my IDE as well, so looking into what's different among the CI, ./refine test, and IntelliJ environments.

[tfmorris]

OK, I'm not sure why the test isn't failing locally (perhaps some difference in the test runner environment?), but the failure is real. I think what's needed is to update the mocks to implement HttpServletRequest.getParameterMap() rather than just HttpServletRequest.getParameter() as they do now

OpenRefine/extensions/database/tests/src/com/google/refine/extension/database/DatabaseImportControllerTest.java

Lines 171 to 178 in cdbdf36

	when(request.getParameter("databaseType")).thenReturn(testDbConfig.getDatabaseType());
	when(request.getParameter("databaseServer")).thenReturn(testDbConfig.getDatabaseHost());
	when(request.getParameter("databasePort")).thenReturn("" + testDbConfig.getDatabasePort());
	when(request.getParameter("databaseUser")).thenReturn(testDbConfig.getDatabaseUser());
	when(request.getParameter("databasePassword")).thenReturn(testDbConfig.getDatabasePassword());
	when(request.getParameter("initialDatabase")).thenReturn(testDbConfig.getDatabaseName());
	when(request.getParameter("query")).thenReturn(query);
	when(request.getParameter("options")).thenReturn(JSON_OPTION);

[wetneb]

The local test suite does not have all tests of the database extension enabled, because those require having a running SQL server (of different flavors).
You can see in the CI configuration how they are enabled:

OpenRefine/.github/workflows/pull_request_server.yml

Lines 99 to 110 in e6dbcfc

	- name: Configure connections to databases
	id: configure_db_connections
	run: cat extensions/database/tests/conf/github_actions_tests.xml | sed -e "s/MYSQL_PORT/${{ job.services.mysql.ports[3306] }}/g" | sed -e "s/POSTGRES_PORT/${{ job.services.postgres.ports[5432] }}/g" > extensions/database/tests/conf/tests.xml
	- name: Populate databases with test data
	id: populate_databases_with_test_data
	run: |
	mysql -u root -h 127.0.0.1 -P ${{ job.services.mysql.ports[3306] }} -proot -e 'CREATE DATABASE test_db;'
	mysql -u root -h 127.0.0.1 -P ${{ job.services.mysql.ports[3306] }} -proot < extensions/database/tests/conf/test-mysql.sql
	psql -U postgres test_db -h 127.0.0.1 -p ${{ job.services.postgres.ports[5432] }} < extensions/database/tests/conf/test-pgsql.sql
	env:
	PGPASSWORD: postgres

[EliasStihl]

Thank you for your responses, but unfortunately, I am not sure how I should move forward with this? I have tried, but I don't know how I should replicate the failures locally so it becomes very hard for me to test.

One small difference between the old URL parser and the new parser is that the old parser took the last value if the key was provided multiple times. However, @tfmorris explained in the issue that parameters could not be repeated. Therefore it shouldn't be a problem to return the first value like this return request.getParameterMap().entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, entry -> entry.getValue()[0])); ?

[tfmorris]

@EliasStihl My apologies. The situation is definitely sub-optimal. The method I used to recreate the failure was to directly run the tests in DatabaseImportControllerTest using my IDE's test runner. I was using IntelliJ, but Eclipse should work as well. Without MySQL installed they won't all succeed, but they should give you enough insight to see what needs to be fixed.

The problem is not with your code, but with the test itself. It mocks getParameter(), but does not mock getParameterMap() which is what the new query parameter parser needs. Because there is other code which still depends on getParameter() they actually both need to be mocked and return the same data, so I'd suggest one or more helper methods be added to the test code.

We apparently don't have the database test environment setup documented for developers, which I will work on now, but you should be able to get away with just MySQL. Basically, you want to:

1. Install MySQL, then run the two commands that @wetneb mentioned above:
2. mysql -u root -h 127.0.0.1 -P <port, if non default> -proot -e 'CREATE DATABASE test_db;'
3. `mysql -u root -h 127.0.0.1 -P -proot < extensions/database/tests/conf/test-mysql.sql
4. Finally, remove this line from the test configuration file

   OpenRefine/extensions/database/tests/conf/tests.xml

   Line 42 in cdbdf36

   <exclude name="requiresMySQL"/>

If you get stuck, or this is too much hassle, let me know and I can fix up the PR so that we can get it merged.

[tfmorris]

Actually, I've got an easier solution. We'll just switch that test to use SQLite instead of MySQL. It doesn't look like I can push to your branch, so I've put the fix up in a branch on my repo. If you git cherry-pick this commit unto your branch, you should get test failures locally. tfmorris@89ee605

[EliasStihl]

Once again, thank you for the incredible support! I think all tests pass now, but please let me know if there is anything you want me to change.

[tfmorris]

Looks great! Thank you for sticking with it! I'll merge it after #6412 gets merged so I can rebase it on top. I also have a couple of test helper methods for DatabaseImportControllerTest which will remove some of the duplication.

For future reference, it makes it easier for us to review if you push incremental fixes in separate commits rather than force pushing a completely new commit. That way we can easily separate stuff that's been reviewed already from the new stuff. All the commits will get squashed when we merge the PR, so there's no need to worry about the extra commits.

[EliasStihl]

Okay, sorry about that, but thanks for telling me so I know how to do it properly in the future!