GH-11248: Make METADATA_VALUE NOT NULL in JdbcMetadataStore schemas - #11259
Conversation
|
Thanks for picking this up — I opened gh-11248. One case I want to make sure I understand: the @artembilan, did "react respectively in the Minor: |
|
yeah — older schemas are out of scope for a runtime guard here. Artem's note was basically "don't let null into the DB": NOT NULL on the shipped scripts + the existing Assert.notNull on put/putIfAbsent. Upgrade path is drop null rows and alter the column (same as the class javadoc). also fixed the DDL sample in |
Right. We will call this out in the Migration Guide. We probably need to double check the logic in that Thanks |
…Store schemas Reject null metadata values at the schema level so putIfAbsent cannot misinterpret a null-valued row as a successful insert. Document the non-null contract (including metadata-store.adoc DDL) and cover it with focused tests. Signed-off-by: Burak KALAYCI <kalayciburak1996@gmail.com>
f9be522 to
390e462
Compare
|
on putIfAbsent: value is already Assert.notNull, so a null arg fails fast with IAE (covered by the new test). return null only means "we inserted". if a legacy row still has a null METADATA_VALUE, the insert path sees the key, queryForObject returns null, and that can look like a successful insert — that's the bug this NOT NULL change is aimed at. new schemas can't store it; old ones need the migrate step you'll put in the Migration Guide. also squashed + fixed DCO sign-off name mismatch on the adoc commit. |
|
Thanks
I guess that is what your AI is doing according to our CONTRIBUTING.md. I'll go ahead and rephrase the rule in the CONTRIBUTING.md: not a first time I hear squash confirmation on subsequent pushes 😄 |
|
got it — won't squash follow-ups. the squash was only to clean a DCO author/sign-off mismatch on the docs commit, not to hide history. next time I'll leave commits as they come and fix sign-off per commit. thanks for clarifying |
artembilan
left a comment
There was a problem hiding this comment.
Add your name to the @author list of the JdbcMetadataStoreTests.
Thank you!
| ---- | ||
|
|
||
| `METADATA_VALUE` is non-null (since 7.2). | ||
| Existing databases created with older scripts should remove any null-valued rows and alter the column to `NOT NULL` when upgrading. |
There was a problem hiding this comment.
Let's move this sentence to the whats-new.adoc!
Create a new section there like:
[[x7.2-jdbc-changes]]
=== JDBC Support Changes
Mention the change you've just did and add a link to the target chapter.
| @Test | ||
| void putRejectsNullValue() { | ||
| assertThatExceptionOfType(IllegalArgumentException.class) | ||
| .isThrownBy(() -> this.metadataStore.put("foo", null)) |
There was a problem hiding this comment.
No foo/bar language, please.
Might be the case that we have a lot in this class already, but that is a different story to be addressed eventually.
Let's just don't introduce more!
Move METADATA_VALUE NOT NULL note to whats-new, drop foo keys in new tests, and add author on JdbcMetadataStoreTests. Signed-off-by: Burak KALAYCI <kalayciburak1996@gmail.com>
|
moved the note to whats-new under JDBC Support Changes, dropped foo keys in the new tests, and added myself to the @author list on JdbcMetadataStoreTests. |
Thanks. Use code snippets in your comments for annotation and classes/methods. See GH docs about commenting style: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax |
Fixes: gh-11248
Summary
JdbcMetadataStore.putIfAbsent()treats a successful read of a SQLNULLMETADATA_VALUEas "insert succeeded" (return null), so a null-valued row can make callers (e.g.MetadataStoreSelector) believe a value was stored when it was not.Per maintainer guidance on the issue, this change makes
METADATA_VALUENOT NULLin all shippedschema-*.sqlscripts for 7.2.0 and documents the non-null contract onJdbcMetadataStore. The store API already rejects null values viaAssert.notNull; the schema now matches that contract.Existing databases created with older scripts should migrate the column to
NOT NULL(and remove any null-valued rows) when upgrading.Test plan
./gradlew :spring-integration-jdbc:test --tests org.springframework.integration.jdbc.metadata.JdbcMetadataStoreTests(12/12)./gradlew :spring-integration-jdbc:checkstyleMain :spring-integration-jdbc:checkstyleTest