Removed parentheses from filtering #256

vbauzys · 2019-10-24T09:40:50Z

Fix for https://github.com/terraform-providers/terraform-provider-vcd/issues/287

Parentheses isn't needed(accordig documentation used when need to create sophisticated filtering) and if we will need to use them later - we have to figure out how to escape them, as in different env behaves differently.

Building query docs: https://pubs.vmware.com/vcloud-api-1-5/wwhelp/wwhimpl/js/html/wwhelp.htm#href=api_prog/GUID-CDF04296-5EB5-47E1-9BEC-228837C584CE.html#1_22_9_1

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

Didainius

Looks to be dead simple.
I was just looking at a more recent version of the docs: https://code.vmware.com/docs/8072/vcloud-director-api-programming-guide/GUID-CDF04296-5EB5-47E1-9BEC-228837C584CE.html

Main concern from me, is the "grouping" (parenthesis) not used so that it matches both conditions (logical and) while without parenthesis would be logical or?

In such case a query with multiple fields may have different results or am I wrong?

This one would look for objects matching both conditions - name to be "name" and orgName to be "adminOrg.AdminOrg.Name":

"filter": fmt.Sprintf("(name==%s;orgName==%s)", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),

While this one would look for objects whose name matches "name" or orgName matches "adminOrg.AdminOrg.Name"

"filter": fmt.Sprintf("name==%s;orgName==%s", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),

dataclouder

A few small changes required. LGTM otherwise

dataclouder · 2019-11-07T14:27:51Z

CHANGELOG.md

@@ -4,6 +4,9 @@
 to allow understand if value was returned or not. 
 * Added method VApp.AddNewVMWithStorageProfile that adds a VM with custom storage profile.

+BUGS FIXED:
+* Removed parentheses from filtering which in some env wasn't treated correctly [#256] (https://github.com/vmware/go-vcloud-director/pull/256)


Suggested change

* Removed parentheses from filtering which in some env wasn't treated correctly [#256] (https://github.com/vmware/go-vcloud-director/pull/256)

* Removed parentheses from filtering since they weren't treated correctly in some environment [#256] (https://github.com/vmware/go-vcloud-director/pull/256)

dataclouder · 2019-11-08T06:03:49Z

govcd/api.go

@@ -122,7 +122,7 @@ func ContainsNotFound(err error) bool {
 	return err != nil && strings.Contains(err.Error(), ErrorEntityNotFound.Error())
 }

-// Function allow to pass complex values params which shouldn't be encoded like for queries. e.g. /query?filter=(name=foo)
+// Function allow to pass complex values params which shouldn't be encoded like for queries. e.g. /query?filter=name=foo


Suggested change

// Function allow to pass complex values params which shouldn't be encoded like for queries. e.g. /query?filter=name=foo

// NewRequestWitNotEncodedParams allows passing complex values params that shouldn't be encoded like for queries. e.g. /query?filter=name=foo

vbauzys · 2019-11-08T06:07:06Z

Looks to be dead simple.
I was just looking at a more recent version of the docs: https://code.vmware.com/docs/8072/vcloud-director-api-programming-guide/GUID-CDF04296-5EB5-47E1-9BEC-228837C584CE.html

Main concern from me, is the "grouping" (parenthesis) not used so that it matches both conditions (logical and) while without parenthesis would be logical or?

In such case a query with multiple fields may have different results or am I wrong?

This one would look for objects matching both conditions - name to be "name" and orgName to be "adminOrg.AdminOrg.Name":
"filter": fmt.Sprintf("(name==%s;orgName==%s)", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),
While this one would look for objects whose name matches "name" or orgName matches "adminOrg.AdminOrg.Name"
"filter": fmt.Sprintf("name==%s;orgName==%s", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),

We can't 100% rely on latest docs as we working still with older versions.
Regarding question Logical OR and Logical AND is managed by separator , and ; .
E.g.

attribute1==value1,attribute2==value2 	
vs 
attribute1==value1;attribute2!=value2

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

dataclouder · 2019-11-08T06:13:45Z

Looks to be dead simple.
I was just looking at a more recent version of the docs: https://code.vmware.com/docs/8072/vcloud-director-api-programming-guide/GUID-CDF04296-5EB5-47E1-9BEC-228837C584CE.html

Main concern from me, is the "grouping" (parenthesis) not used so that it matches both conditions (logical and) while without parenthesis would be logical or?

In such case a query with multiple fields may have different results or am I wrong?

This one would look for objects matching both conditions - name to be "name" and orgName to be "adminOrg.AdminOrg.Name":
"filter": fmt.Sprintf("(name==%s;orgName==%s)", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),
While this one would look for objects whose name matches "name" or orgName matches "adminOrg.AdminOrg.Name"
"filter": fmt.Sprintf("name==%s;orgName==%s", url.QueryEscape(name), url.QueryEscape(adminOrg.AdminOrg.Name)),

Parentheses are needed only if we have groups, or sub-expression.
For example, "(A=1;B=2),(A=2;B=3)" means (A == 1 AND B == 2) OR (A ==2 AND B == 3)

If we have only one set of parentheses, they should be superfluous, as we are grouping the only sub-expression in the condition.
(Notice that in your example the comma should not be followed by spaces)

Didainius · 2019-11-08T06:16:59Z

Parentheses are needed only if we have groups, or sub-expression.
For example, "(A=1;B=2),(A=2;B=3)" means (A == 1 AND B == 2) OR (A ==2 AND B == 3)

If we have only one set of parentheses, they should be superfluous, as we are grouping the only sub-expression in the condition.
(Notice that in your example the comma should not be followed by spaces)

Alright. I am sold :)

The example in website confused me initially having and everywhere:

(attribute1==value1;attribute2!=value2);(attribute3==value3;attribute4!=value4)...

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

lvirbalas

LGTM, but in the commit message, please explain why these parentheses were doing harm - I don't see a single note about that in this PR.

Removed parentheses from filtering

26903da

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

vbauzys self-assigned this Oct 24, 2019

vmwclabot added the cla-not-required label Oct 24, 2019

Removing left parenthesis

d159261

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

vbauzys added the draft label Oct 30, 2019

vbauzys requested review from dataclouder, lvirbalas and Didainius and removed request for dataclouder and lvirbalas November 7, 2019 13:40

vbauzys added 2 commits November 7, 2019 15:43

Merge branch 'master' into fix_filtering

50f6dcf

Update changelog

cecc7fe

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

vbauzys removed the draft label Nov 7, 2019

Didainius reviewed Nov 8, 2019

View reviewed changes

dataclouder approved these changes Nov 8, 2019

View reviewed changes

Improve comments

bf6a5cc

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

Didainius approved these changes Nov 8, 2019

View reviewed changes

Improve comments

3622605

Signed-off-by: Vaidotas Bauzys <vbauzys@vmware.com>

lvirbalas approved these changes Nov 8, 2019

View reviewed changes

vbauzys merged commit 5cf7034 into vmware:master Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed parentheses from filtering #256

Removed parentheses from filtering #256

vbauzys commented Oct 24, 2019 •

edited

Loading

Didainius left a comment

dataclouder left a comment

dataclouder Nov 7, 2019

vbauzys Nov 8, 2019

dataclouder Nov 8, 2019

vbauzys Nov 8, 2019

vbauzys commented Nov 8, 2019 •

edited

Loading

dataclouder commented Nov 8, 2019

Didainius commented Nov 8, 2019

lvirbalas left a comment

	* Removed parentheses from filtering which in some env wasn't treated correctly [#256] (https://github.com/vmware/go-vcloud-director/pull/256)
	* Removed parentheses from filtering since they weren't treated correctly in some environment [#256] (https://github.com/vmware/go-vcloud-director/pull/256)

	// Function allow to pass complex values params which shouldn't be encoded like for queries. e.g. /query?filter=name=foo
	// NewRequestWitNotEncodedParams allows passing complex values params that shouldn't be encoded like for queries. e.g. /query?filter=name=foo

Removed parentheses from filtering #256

Removed parentheses from filtering #256

Conversation

vbauzys commented Oct 24, 2019 • edited Loading

Didainius left a comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

dataclouder Nov 7, 2019

Choose a reason for hiding this comment

vbauzys Nov 8, 2019

Choose a reason for hiding this comment

dataclouder Nov 8, 2019

Choose a reason for hiding this comment

vbauzys Nov 8, 2019

Choose a reason for hiding this comment

vbauzys commented Nov 8, 2019 • edited Loading

dataclouder commented Nov 8, 2019

Didainius commented Nov 8, 2019

lvirbalas left a comment

Choose a reason for hiding this comment

vbauzys commented Oct 24, 2019 •

edited

Loading

vbauzys commented Nov 8, 2019 •

edited

Loading