camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.09k stars 1.54k forks source link

Queries containing lower case column names fail with case-sensitive database collation #4525

Closed danielkelemen closed 3 days ago

danielkelemen commented 1 month ago

Environment (Required on creation)

7.20, 7.21, master.

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket)

There are two queries containing lower case column names. If they are run in a case-sensitive database, for instance SQL Server with Latin1_General_100_CS_AS collation, then the query fails.

Steps to reproduce (Required on creation)

  1. Start Camunda with SQL Server and Latin1_General_100_CS_AS collation.
  2. Open cockpit and the process definition search fails.

Observed Behavior (Required on creation)

Process definition search fails.

Expected behavior (Required on creation)

Process definition search works.

Root Cause (Required on prioritization)

The tables are created with upper case column names and these queries contain lower case column names. By default databases are case insensitive, but if they are started with case-sensitive collation like SQL Server Latin1_General_100_CS_AS, then the database server can not find the column. (SQLServerException: Ungültiger Spaltenname "version_".)

Solution Ideas

Hints

Queries: https://github.com/search?q=repo%3Acamunda%2Fcamunda-bpm-platform+%2F%28%3F-i%29%5C.%5Ba-z_%5D*_%2F+language%3Axml&type=code

Links

Breakdown

### Pull Requests
- [ ] https://github.com/camunda/camunda-bpm-platform/pull/4542
- [ ] https://github.com/camunda/camunda-bpm-platform-maintenance/pull/1275
- [ ] https://github.com/camunda/camunda-bpm-platform-maintenance/pull/1274
- [ ] https://github.com/camunda/team-automation-platform/issues/181

Dev2QA handover

mboskamp commented 3 weeks ago

Decision: We decided that a dedicated CI with a case-sensitive database is unnecessary. Instead, we opted for tests that scan the SQL mapping files for lower-case usages.

I duplicated the test for the webapps. It felt overkill to expose engine test-sources to the webapps just for abstracting the scan logic in a utility class.

tasso94 commented 3 weeks ago

We decided that a dedicated CI with a case-sensitive database is unnecessary.

I wouldn't say it's unnecessary. It's a bit overkill, given we only officially support SQL Server with its default configuration, which uses the case-insensitive CI database collation. The test you added is a good trade-off between establishing a safety net to avoid running into similar issues in the future again and time investment. On the downside, there might be edge cases we cannot detect with this test. E.g., introducing table names/columns with this problem.

If a customer explicitly asks us to support SQL Server configured with collation CS by opening a feature request, I would add a dedicated CI stage with a correctly configured SQL Server to test the behavior. We would then also need to document this.

mboskamp commented 3 weeks ago

@tasso94, I agree with your comment about the testing approach. I meant this when I said adding an extra CI is unnecessary. Thanks for giving more background information on why we won't add an extra CI now. 👍

mboskamp commented 1 week ago

Reopen to add tests for EE webapps.

mboskamp commented 3 days ago

For the initial implementation, we decided not to cover the table names with tests and focus on the column names. Reason: It isn't easy to create a regex that matches both reliably and we can iterate to cover table names in the future.

mboskamp commented 3 days ago

Follow-up ticket to ensure table names are upper-case: https://github.com/camunda/camunda-bpm-platform/issues/4591