-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(entity controller) Fix case sensitivity in entity controller #14902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(entity controller) Fix case sensitivity in entity controller #14902
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. 📢 Thoughts on this report? Let us know! |
| for (String entityName : entityTypes) { | ||
|
|
||
| // Case-insensitive matching. | ||
| List<IngestResult> entityResults = resultsByEntityType.getOrDefault(entityName, List.of()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhixuanjia we can always check for lower case here ?
cc: @david-leifker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that something about this feels off. Above we're setting the entries in the map to be the direct entityType from the urn (which should not be lowercase, it would be cased in the way the entity is defined, usually camelCase). Entity types are technically case sensitive (practically not, because of some auto lowercasing in entity registry, but not by the urn itself). I would expect consistency between how the keys are in the map are being set up and this code here. Either we should be consistently lowercasing them in the map above or it shouldn't need this lowercase call here (calling both of the getOrDefaults feels even weirder).
Would like to see tests exemplifying this case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the urn (which should not be lowercase, it would be cased in the way the entity is defined, usually camelCase).
@RyanHolstien to provide some context here, we are not using the standard Datahub entity types like dataset, dataJob etc. Our models are loaded as plugins and they are all lowercased. And we use Open API spec to create Rest clients to interact with this endpoint. In the Open API spec, the entity names are from entity registry and they are in camelCases but entity types are all lowercased.
There are a few options we could fix this:
-
We could change all our entity types to camelCases to solve this issue but it's just too much work on our side. It will requires data migration.
-
We could also change the open api spec generator to use lowercases. But that will change the Rest client / interface which our other services have dependencies on.
-
Case-insensitive matching so that "FooBar" and "foobar" are considered the same entity type. This is the option used in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wouldn't it make more sense for your entity types in the registry to be the same casing as it is for your actual entities? I.e. just change your plugin entity-registry.yml to match what the entities actually have. This kind of feels like putting customization specific code into the main OSS trunk the way you've described it 🤔
Head branch was pushed to by a user without write access
bc04288 to
0e93f1d
Compare
.../openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java
Show resolved
Hide resolved
| for (String entityName : entityTypes) { | ||
|
|
||
| // Case-insensitive matching. | ||
| List<IngestResult> entityResults = resultsByEntityType.getOrDefault(entityName, List.of()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| List<IngestResult> entityResults = resultsByEntityType.getOrDefault(entityName, List.of()); | |
Then remove this second check
5cd26d4 to
6ba1bfb
Compare
) Co-authored-by: jjia <[email protected]>
Issue Summary
Currently in the request body of
createGenericEntitiesAPI , the entity name is passed in as camel case likeFooBarbut the entity type stored in DB is lowercase such asurn:li:foobar:123. This results in empty list in the response.Fix
Perform case-insensitive matching on the result map key.