beatum commented on PR #8339:
URL: https://github.com/apache/hop/pull/8339#issuecomment-5709477739
Thanks for the detailed review. I have addressed the feedback with the
following changes.
## 1. SQL parameter parsing
Updated `Database.parseSqlParameterSpec()` so that it no longer infers JSONB
operators from surrounding identifier-like characters.
The parser now skips only these explicit PostgreSQL/JDBC operator tokens:
- `?|`
- `?&`
- `??`
- `??|`
- `??&`
It consumes the complete operator token, so an expression such as:
```sql
col ?? ?
```
correctly treats `??` as the operator and the trailing `?` as a bind
parameter.
I also added the following overload:
```java
parseSqlParameterSpec(String sql, boolean consumeBracketIdentifiers)
```
The default static parser uses `consumeBracketIdentifiers = false`, which is
safer for general SQL. Bracketed content, such as `[identifier]`, is consumed
as an identifier only when the flag is enabled.
## 2. Dialect-aware bracket handling
`Database.countParameters()` now enables bracket-identifier parsing only
when the dialect uses `[` and `]` as its identifier quote characters.
This preserves SQL Server-style bracket identifiers while allowing
PostgreSQL array expressions such as the following to be parsed correctly:
```sql
ANY(ARRAY[?])
ARRAY[?, ?]
```
`DatabaseJoinMeta.parseSqlParameterSpec()` now provides the same
bracket-handling overload, and the runtime call sites pass the appropriate
setting from `DatabaseMeta`.
## 3. `DatabaseJoinMeta#getTableFields()` error handling
Updated `DatabaseJoinMeta#getTableFields()` so that stored-procedure
metadata lookup is deferred only when `db.getQueryFields(...)` fails after a
successful database connection.
The `db.connect()` call remains outside the stored-procedure deferral path.
As a result:
- Connection failures are no longer swallowed as "defer to runtime."
- Errors from non-procedure SQL are propagated normally.
- Stored-procedure SQL can still defer metadata resolution when the metadata
query itself fails.
This aligns `getTableFields()` with the safer behavior already used by
`getFields()`.
## 4. Regression tests
Updated and expanded `DatabaseTest` coverage for:
- A lone `?` in `payload ? 'key'`
- `LIKE ? AND`
- `NOT LIKE ?`
- `BETWEEN ? AND ?`
- `SELECT ? FROM dual`
- `col ?? ?`
- Escaped JSONB operator tokens
- `ANY(ARRAY[?])`
- `ARRAY[?, ?]`
- Dialect-specific bracket identifier handling
- Correct parameter counts when SQL tokenizer rules are applied
Updated `DatabaseJoinMetaTest` coverage for:
- PostgreSQL array constructors containing positional parameters
- Explicit JSONB operator token handling
- Dialect-aware bracket identifier behavior
## 5. Database Join dialog
Updated `DatabaseJoinDialog` with a read-only **Resolved SQL parameters**
table directly below the SQL editor.
The table is populated through the same
`DatabaseJoinMeta.parseSqlParameterSpec(...)` path used by runtime parsing and
displays:
- Parameter index
- Placeholder, such as `?` or `?{name}`
- Resolved input field
- Field type
The table updates whenever any of the following changes:
- SQL text
- Connection
- SQL-file mode
- Parameter mappings
Unresolved mappings are displayed inline as `not found` or `not mapped`.
The existing positional-parameter grid is also disabled when the SQL
contains no bare positional `?` placeholders.
I added the required English localization keys to
`messages_en_US.properties`.
<img width="721" height="617" alt="image"
src="https://github.com/user-attachments/assets/ca596d52-a697-47b8-a9d5-695a0a7a979b";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
