`JdbcMetadataStore.putIfAbsent()` returns `null` for an existing row with a null `METADATA_VALUE`

[lsh1215]

Split out from #10847 / #11247, where @artembilan asked for this to be its own story.

ConcurrentMetadataStore.putIfAbsent documents its return value as:

@return null if successful, the old value otherwise.

METADATA_VALUE is nullable in every shipped schema (schema-mysql.sql, schema-derby.sql, and the rest), so a row can exist with a null value. For such a row JdbcMetadataStore.putIfAbsent() returns null: the insert reports 0 affected rows because the row is there, and queryForObject returns null rather than throwing EmptyResultDataAccessException. Nothing was stored, yet the caller is told it was.

MetadataStoreSelector reads the return value literally:

return this.metadataStore.putIfAbsent(key, value) == null;

and its javadoc says a null return "means that the value has been placed in the MetadataStore". So an idempotent receiver keyed on a null-valued row accepts every duplicate and never records anything.

Reproducing it needs no concurrency:

new JdbcTemplate(dataSource)
        .update("INSERT INTO INT_METADATA_STORE(METADATA_KEY, METADATA_VALUE, REGION) VALUES (?, ?, ?)",
                "nullValued", null, "DEFAULT");

// returns null, as if the value had just been stored
metadataStore.putIfAbsent("nullValued", "someValue");

The obvious fix is not obvious, which is why I did not fold it into #11247. putIfAbsent() distinguishes "row absent" from "row present with a null value" only by whether queryForObject throws, and the retry loop relies on that distinction to terminate. Collapsing the two cases would leave the loop with no exit. Options I can see:

• Make the column non-nullable, which is a schema change for existing users.
• Reject null values on write, since put() and putIfAbsent() already Assert.notNull the value - a null can only get there out of band.
• Have the store treat a null-valued row as present and return something other than null, which the interface's return type does not allow without changing the contract.

Happy to work on whichever direction you prefer, or to leave it as documentation if you consider a null METADATA_VALUE unsupported in the first place.

[artembilan]

I think we just have to bite a bullet and don't let the null to slip into DB!
So, made that METADATA_VALUE column as NOT NULL, and react respectively in the JdbcMetadataStore.
Yes, this is some kind of breaking change, but that is OK for our current new 7.2.0 minor.

Let us know if you are willing to contribute the fix!

Thank you!

[kalayciburak]

I can take this for 7.2.0: make METADATA_VALUE NOT NULL across the shipped schemas and reject null values in JdbcMetadataStore with focused tests. Will open a PR shortly.