Allow dashes in user urn #1564
Allow dashes in user urn #1564
Conversation
Update to lateest version of DataHub
CorpUserInfo urns containing dashes are valid entries. When adding that user as an owner, the mae job validates the owner's urn using a regex filter that only accepts alphanumeric characters and underscore (\w). That means any ownership changes where the user urn contains an underscore are rejected. This change extends the regex filter to allow dashes in the name. It includes unit tests that verify the change works for multiple dashes and underscores. There are other cases to consider: 1. Should any other characters be allowed? 2. Should the filter check the urn starts and ends with alphanumeric characters? CLOSES: User urn does not handle dashes consistently #1554 BREAKING CHANGE: None. This change relaxes a restriction so existing code is ok.
| @@ -24,9 +24,11 @@ public void testGetCorpUserOwners() { | |||
| Owner owner1 = new Owner().setOwner(new CorpuserUrn("t1")); | |||
| Owner owner2 = new Owner().setOwner(new CorpuserUrn("t2")); | |||
| Owner owner3 = new Owner().setOwner(new CorpGroupUrn("t3")); | |||
| List<Owner> owners = Arrays.asList(owner1, owner2, owner3); | |||
| Owner owner4 = new Owner().setOwner(new CorpGroupUrn("t-4-t")); | |||
There was a problem hiding this comment.
Maybe change this to CorpuserUrn to test both urn types?
There was a problem hiding this comment.
I added tests or urn and groups. Adjusted the results to match what should be the correct values. These tests do not appear to run when building the project though. All the unit tests report success regardless of the data entered here. The tests are built, just not run.
| ownership.setOwners(new OwnerArray(owners)); | ||
| assertEquals(BuilderUtils.getCorpUserOwners(ownership), Arrays.asList("t1", "t2")); | ||
| assertEquals(BuilderUtils.getCorpUserOwners(ownership), Arrays.asList("t1", "t2", "t3", "t-4-t", "t_5_t")); |
There was a problem hiding this comment.
getCorpUserOwners should only return owners which are users not groups. I'm really curious how this test passed 😮
There was a problem hiding this comment.
Looking through the logs, I can't tell that these tests are getting executed. I see lots of other tests being executed, but this assertion does not fire no matter what values are in the list.
The problem with the test not running seems like a different issue. I corrected the test results, but not sure what triggers the test execution or why this test is not firing.
|
@ben5448 Thanks for this PR. It helped me figure out an underlying issue with some modules not being tested at all. I'll push the fix soon. I will just accept this now. |
fix: allow dashes in user urn
Dashes are accepted in user urns for the CorpUserInfo documents. In the MAE
consumer for ownership facets, the owner urn is validated using a regex that
only accepts alphanumeric characters and underscores. If the owner's urn
contains a dash, that update triggers an error.
This fix: relax regex validation to allow a dash as a valid character in the urn.
Unit tests changes match verify the new filter and verify the filter allows
multiple dashes or underscores. No attempt is made for force the first or
last characters to be alphanumeric or to allow other characters.
CLOSES: User urn does not handle dashes consistently #1554
BREAKING CHANGES: None. This change relaxes a constraint so all existing
code should continue to work the same.