Ignore empty entries in org domain list#423
Conversation
| private String url; | ||
|
|
||
| @JsonProperty("domains") | ||
| @JsonDeserialize(contentUsing = EmptyStringAsNullDeserializer.class) |
There was a problem hiding this comment.
Does anyone remember why there are domains in the attributes?
There was a problem hiding this comment.
Two tests in OrganizationImportTest broke without this at some point, 'cause to my understanding the OrganizationsResource::importOrgs method parses the request's body eventually to this POJO.
But if I remove these two lines, no tests break at the moment. Give me a little time to understand better, if the annotations in the class io.phasetwo.service.representation.Organization are enough, or we really need these.
There was a problem hiding this comment.
Give me a little time to understand better, if the annotations in the class io.phasetwo.service.representation.Organization are enough, or we really need these.
I think we do need them on the OrganizationAttributes class. When the annotation is not present, it seems that an empty string gets persisted to the database, but is later filtered out when the data is returned via the API. I need to think a bit about how we can best write tests to cover this behavior.
Related to that filtering - do we mind that after this change the API behavior will change slightly? Already saved empty strings will no longer be returned in the domain list. I don’t think it’s a problem, because if someone queries and saves the org representation, the empty strings would be removed anyway. With this API change, the difference on the UI should be minimal: users won’t see the empty strings even before saving.
There was a problem hiding this comment.
When the annotation is not present, it seems that an empty string gets persisted to the database, but is later filtered out when the data is returned via the API.
This is indeed the case. I added a new test class, OrganizationImportWithDatabaseAccessTest, which starts a Keycloak instance backed by a PostgreSQL Testcontainers database. The test asserts that no empty strings are persisted to the ORGANIZATION_DOMAIN table.
If the two newly added annotations are removed from the OrganizationAttributes class, the OrganizationImportWithDatabaseAccessTest test fails, demonstrating the issue.
vilmosnagy
force-pushed
the
403-ignore-empty-entries-in-org-domain-list
branch
from
579b0b1 to
ae819d6
Compare
vilmosnagy
force-pushed
the
403-ignore-empty-entries-in-org-domain-list
branch
from
cda695a to
f0f27f4
Compare
vilmosnagy
force-pushed
the
403-ignore-empty-entries-in-org-domain-list
branch
from
f0f27f4 to
ec69421
Compare
Code Coverage
|
2 participants
Closes #403