Skip to content
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] Discovery middleware shouldn't fail when lib names overlap #362

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

codeworrior
Copy link
Member

When a library's name is a prefix of another library's name (like with
sap.m and sap.me), the discovery service mistakenly assigned test pages
to both libraries.

By including the slash in the prefix check, only full segment matches
will be taken into account.

By checking the prefixes from longest to shortest and by aborting after
the first match, non-prefix-free lib names can be handled properly. The
longest matching prefix will win and there won't be double assignments.

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

When a library's name is a prefix of another library's name (like with
sap.m and sap.me), the discovery service mistakenly assigned test pages
to both libraries.

By including the slash in the prefix check, only full segment matches
will be taken into account.

By checking the prefixes from longest to shortest and by aborting after
the first match, non-prefix-free lib names can be handled properly. The
longest matching prefix will win and there won't be double assignments.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 93.506% when pulling 7e48e73 on fix-discovery into d9a3ce6 on master.

Copy link
Contributor

@svbender svbender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the priority of this? Should be release it rather quickly?

@codeworrior
Copy link
Member Author

@RandomByte from my POV not urgent. Just stumbled upon redundant entries in the testsuite app's tree when running it in a scope where sap.m and sap.me both are present. But all entries can be found and executed, so it's not a functional issue.

@codeworrior codeworrior merged commit f5067ce into master Sep 24, 2020
@codeworrior codeworrior deleted the fix-discovery branch September 24, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants