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 regexp path patern's not all checked #670

Conversation

stan-at-work
Copy link
Contributor

@stan-at-work stan-at-work commented Jul 23, 2024

Summary

Issue: Regular expression (regexp) path patterns are not being fully checked during the path pattern matching process.

Detailed Description

I encountered this issue while working with path pattern matching. The core problem lies in the handling of regular expression (regexp) path patterns within the codebase. Specifically, the current implementation fails to iterate over all provided path patterns, stopping the process prematurely when it encounters a failure. This limitation significantly restricts the utility and flexibility of using regular expressions for path matching.

Solution

The proposed fix ensures that all path patterns are checked by modifying the iteration logic. Instead of returning upon the first failure, the code now continues to iterate through the entire list of path patterns, thereby improving the robustness and reliability of the regexp path pattern matching.

By applying this fix, the system will be able to handle a more extensive range of path patterns, thus enhancing its functionality and accommodating more complex use cases.

…lways return.

If there are 4 path pattern's and all of them are regex, then there was a bug that only the first one was checked. if that one failed then all the other ones where skipped. this fixes that.
@stan-at-work stan-at-work changed the title Fix regexp path patern not all checked Fix regexp path patern's not all checked Jul 23, 2024
@slovnicki
Copy link
Owner

slovnicki commented Jul 28, 2024

Oh wow, what a find!
I'm surprised how this was missed and not noticed sooner. (probably not a lot of people using regex path patterns, me included)

Thank you very much for the PR! 💙

Before merging, I would like to take more time for these 2 things;

  • figure out why we seem to have a duplication of that logic in both beam_guard and utils, and can we alleviate that
  • write at least 1 test for this

@stan-at-work
Copy link
Contributor Author

stan-at-work commented Jul 29, 2024

Oh wow, what a find!
I'm surprised how this was missed and not noticed sooner. (probably not a lot of people using regex path patterns, me included)

Thank you very much for the PR! 💙

Before merging, I would like to take more time for these 2 things;

  • figure out why we seem to have a duplication of that logic in both beam_guard and utils, and can we alleviate that
  • write at least 1 test for this

Point 1

  • year, I noticed that to. That was indeed a bit weard.

I think its. Best to put it all in the Utils class.

@slovnicki
Copy link
Owner

I think its. Best to put it all in the Utils class.

I agree, let's try to refactor this to be only in utils.

@Sten435 Sten435 deleted the fix-regexp-path-patern-not-all-checked branch August 4, 2024 16:03
@Sten435
Copy link
Contributor

Sten435 commented Aug 4, 2024

@slovnicki I created a new PR with some changes and new issues fixed.

#674

@Sten435
Copy link
Contributor

Sten435 commented Aug 4, 2024

Closing this, as it has no value anymore

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.

3 participants