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

wildcard: Fix wildcard match behavior to support wide range. #7

Merged
merged 1 commit into from Aug 24, 2016
Merged

wildcard: Fix wildcard match behavior to support wide range. #7

merged 1 commit into from Aug 24, 2016

Conversation

harshavardhana
Copy link
Contributor

This helps in supporting all types of patterns in wildcard match.

This helps in supporting all types of patterns in wildcard match.
@harshavardhana
Copy link
Contributor Author

harshavardhana commented Aug 24, 2016

@tidwall - 'wildcard' match treats the input string as flat here. Let me know what you think about this.

@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

@harshavardhana Thanks for the PR. What do you mean by wide range and all types of patterns? Is there a bug in the current implementation?

@harshavardhana
Copy link
Contributor Author

@harshavardhana Thanks for the PR. What do you mean by wide range and all types of patterns? Is there a bug in the current implementation?

Yes there are cases where the current implementation doesn't work, you can use the current code against the range of tests submitted to see why they fail.

@harshavardhana
Copy link
Contributor Author

For example

    match_test.go:308: Test 2, params: {my-folder/In* my-folder/India/Karnataka/ true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 4, params: {my-folder/In*/Ka*/Ban my-folder/India/Karnataka/Ban true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 5, params: {my-folder/In*/Ka*/Ban my-folder/India/Karnataka/Ban/Ban/Ban/Ban/Ban true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 6, params: {my-folder/In*/Ka*/Ban my-folder/India/Karnataka/Area1/Area2/Area3/Ban true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 7, params: {my-folder/In*/Ka*/Ban my-folder/India/State1/State2/Karnataka/Area1/Area2/Area3/Ban true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 9, params: {my-folder/In*/Ka*/Ban* my-folder/India/Karnataka/Bangalore true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 10, params: {my-folder/* my-folder/India true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 29, params: {my-folder/mnop*? my-folder/mnopqrst/mnopqr true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 30, params: {my-folder/mnop*? my-folder/mnopqrst/mnopqrs true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 32, params: {my-folder/mnop*? my-folder/mnopq true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 33, params: {my-folder/mnop*? my-folder/mnopqr true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 34, params: {my-folder/mnop*?and my-folder/mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 36, params: {my-folder/mnop*?and my-folder/mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 38, params: {my-folder/mnop*? my-folder/mnopqrst/mnopqrs true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 39, params: {my-folder/mnop*?? my-folder/mnopqrst true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 40, params: {my-folder/mnop*qrst my-folder/mnopabcdegqrst true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 41, params: {my-folder/mnop*?and my-folder/mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:308: Test 43, params: {my-folder/mnop*?and? my-folder/mnopqanda true}: Expected the result to be `true`, but instead found it to be `false`

BTW Note this PR does a flat key match (i.e delimiters like / are not treated specially), so i am wondering if that is what you are expecting?

@harshavardhana
Copy link
Contributor Author

Other simplified version of strings which fail with current implementation.

    match_test.go:309: Test 29, params: {mnop*? mnopqrst/mnopqr true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 30, params: {mnop*? mnopqrst/mnopqrs true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 32, params: {mnop*? mnopq true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 33, params: {mnop*? mnopqr true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 34, params: {mnop*?and mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 36, params: {mnop*?and mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 38, params: {mnop*? mnopqrst/mnopqrs true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 39, params: {mnop*?? mnopqrst true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 40, params: {mnop*qrst mnopabcdegqrst true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 41, params: {mnop*?and mnopqand true}: Expected the result to be `true`, but instead found it to be `false`
    match_test.go:309: Test 43, params: {mnop*?and? mnopqanda true}: Expected the result to be `true`, but instead found it to be `false`

tidwall added a commit that referenced this pull request Aug 24, 2016
@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

I'm not seeing the same issue on my side. I created a branch named bad-match that pulls from the same branch that you did. I added your test code to the gjson_test.go file and it works for me.
One thing that I had to do was switch the order of the params in your test from wildcardMatch(pattern,key) to wildcardMatch(key,pattern).

Could you check it on your side?

@harshavardhana
Copy link
Contributor Author

Could you check it on your side?

Will do thanks @tidwall

@harshavardhana
Copy link
Contributor Author

@tidwall looks like you are right.. must be some problem on my end. when i changed the argument order. Let me rip off my implementation and just submit those tests.. ? what do you think?

@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

Sounds good. Thanks.

@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

Actually, I can merge your changes locally and strip off the code, then merge it to master. I don't think you need to do anything.

@tidwall tidwall merged commit 0608a38 into tidwall:master Aug 24, 2016
@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

I just added your test to the gjson_test.go file.

@harshavardhana harshavardhana deleted the wildcard-match branch August 24, 2016 23:56
@harshavardhana
Copy link
Contributor Author

I just added your test to the gjson_test.go file.

Thanks @tidwall

@tidwall
Copy link
Owner

tidwall commented Aug 24, 2016

You're welcome and thanks a ton for supporting the project.

@harshavardhana
Copy link
Contributor Author

You're welcome and thanks a ton for supporting the project.

This is great work does a big win for us @minio

@tidwall
Copy link
Owner

tidwall commented Aug 25, 2016

Sweet! Your team is making some pretty cool stuff over there.

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.

None yet

2 participants