Does that mean we need to store nulls in note values?

note: #9741

Yes, this is something that came up while running the testsuite... in some scenario one of the notes had only the key, but a null value, and NPE was seen as a result. I don't recall exactly what option was that and perhaps the right way to fix it would be to change the code that adds the notes so it doesn't add a note if it has no value.

perhaps the right way to fix it would be to change the code that adds the notes so it doesn't add a note if it has no value.

+1 for this solution

+1 to fixing the code where null is put, but not in this PR. Still, we need to be defensive here, so even if there is null value then the code needs to work.

Think of no-downtime upgradability - if one version supported null and the other did not, the code must not fail with NPE.

Explanation and workaround is summarized e.g. in https://stackoverflow.com/a/24632080/6930869

I'll leave this code as is then, and open a separate tracker to fix the inclusion of a null value - need to find the test that was failing.

Please paste a link to the separate issue here once it's created, thank you.

I've tracked the failing tests and the null values are usually either the scope or nonce in OAuth2DeviceCodeModel. Opened #12774.

I was thinking about being defensive - wouldn't it be better if we didn't store null-valued notes in the first place? In other words, adding null checks in setNote() and setNotes to discard non-null notes. Then getNotes could safely use Collectors::toMap knowing that null values do not exist in the store.

Edit: this comment is not relevant as JPA has its own implementation of these methods

Hmm, looking into setNote and setNotes method implementation:

@SuppressWarnings("unchecked") @Override public void setNotes(java.util.Map<String,String> p0) {
        p0 = p0 == null ? null : new java.util.HashMap<>(p0);
        if (p0 != null) {
            p0.values().removeIf(org.keycloak.models.map.common.UndefinedValuesUtils::isUndefined);
            if (org.keycloak.models.map.common.UndefinedValuesUtils.isUndefined(p0)) p0 = null;
        }
        updated |= ! Objects.equals(fNotes, p0);
        fNotes = p0;
    }

@SuppressWarnings("unchecked") @Override public void setNote(String p0, String p1) {
        boolean valueUndefined = org.keycloak.models.map.common.UndefinedValuesUtils.isUndefined(p1);
        if (valueUndefined) { if (fNotes != null) { updated |= fNotes.remove(p0) != null; } return; }
        if (fNotes == null) { fNotes = new java.util.HashMap<>(); }
        Object v = fNotes.put(p0, p1);
        updated |= ! Objects.equals(v, p1);
    }

this should already remove undefined values.

For the record: I've used exactly it in user/cllient sessions: https://github.com/keycloak/keycloak/pull/12241/files#diff-f8c7267d81dd0ea60117e905f425105d651dda638b403d2c06e80b3eed5c5cceR302

I'll follow the same path then, and have single-use object notes consistent with the user session notes.

Please paste a link to the separate issue here once it's created, thank you.