Query enhancement and filter engine for metadata search #298

dataclouder · 2020-04-03T06:15:17Z

Added methods catalog.QueryVappTemplateList, catalog.QueryCatalogItemList, catalog.QueryWithMetadataFields, catalog.QueryWithMetadataFilter to support search by filter and filter by metadata in terraform-provider-vcd

New query engine based on client.SearchByFilter, type FilterDef, and interface QueryItem.

* Change SearchByFilter function to use dependency injection * Add unit test for query engine * Add filter type 'earliest' * Add human readable explanation of engine actions

Add filter_helpers.go, containing functions used to create filters

Queries that are paginated will iterate until all elements are collected.

Add filed type to metadata contents

If we use `xml:"type"`, we get the metadata info, but then the metadata update will fail. We keep it as it is for the time being

* Add more error information * Add guessing of metadata type when API doesn't return the info

Didainius

Pass one.
Great PR! As a side joke I wished writing YOLO and approving a few times :)

Getting to serious matters - a few more thoughts in addition to comments inline:

One thing which I'd like to change is conditionMatches() function and avoid types for each field like dateCondition, ipCondition, etc. In reality they are just two types string or *regexp.Regexp. However so far a perfect solution doesn't come to my mind so far so have nothing exact to propose and will obey.
var definitions in searchByFilter could come closer to their usage

Test suites passed on 9.5 and 10.1.

govcd/adminorg.go

govcd/filter_engine.go

Didainius · 2020-05-05T08:19:02Z

govcd/filter_engine.go

+	if err != nil {
+		return nil, explanation, fmt.Errorf("[SearchByFilter] error retrieving query item list: %s", err)
+	}
+	if os.Getenv("GOVCD_FAIL") == "1" {


A thought here - wouldn't it be the same in terms of debugging to simply use logging utility and dump structures to the log. That way we could tap into existing troubleshooting system. No need to ask users for additional env variable setting.

The approach was made this way because the output of these user-defined failures are huge. If we use them always, the log will grow considerably.
A change that I am considering is to make these logs entries depend on GOVCD_FAIL=X.

As I mentioned in my intro presentation, I could completely eliminate this backdoor once we finish the review, or we may decide to convert them into a log-based interface

Having them is good. In some situations we need all we can get :)
Another thought about logging. What if we gradually introduce loglevels to GOVCD_LOG variable. For example log this output only when GOVCD_LOG=TRACE. It wouldn't break anything, but could give flexibility without new variables.

Changed to a log-based data inspection. Also explained in CODE_GUIDELINES

govcd/filter_util.go

govcd/query_metadata.go

govcd/filter_engine_test.go

govcd/filter_util.go

govcd/query_metadata.go

govcd/filter_condition.go

vbauzys

That's huge work. Thank you
First things which I would like to be improved.

vbauzys · 2020-05-05T12:56:56Z

CODING_GUIDELINES.md

+
+* If we want to include metadata, we need to add the metadata fields to the list of fields we want the query to fetch.
+* Unfortunately, not all fields defined in the corresponding type is accepted by the `fields` parameter in a query.
+The fields returned by `queryFieldsOnDemand` are the one that have been proven to be accepted.


Here, to better understand any example would be valuable

Please see the comment at the top of the file.

vbauzys · 2020-05-05T13:00:33Z

CODING_GUIDELINES.md

+                },
+                UseMetadataApiFilter: false,
+            }
+			queryType := govcd.QtMedia


Not sure about naming QtSomething

Qt stands for "Query Type". It's used for constants that can be called from go-vcloud-director and terraform provider, to avoid mistyping. For example, vApp template query types use a bizarre camel case that is easy to get wromg.

govcd/adminorg.go

govcd/catalogitem.go

govcd/filter_condition.go

govcd/filter_engine.go

govcd/filter_helpers.go

When the catalog items are more than the default page size, we don't get all of them. Instead of calling the regular QueryWithNotEncodedParams, we need to run cumulativeQuery

* Improve functions comments * Add to CHANGELOG.md * Make some functions private * Reduce wide scope variable in the search engine

lvirbalas · 2020-05-12T11:18:08Z

govcd/vdc_test.go

@@ -112,7 +112,7 @@ func (vcd *TestVCD) Test_NewVdc(check *C) {

 	// Skipping this check, as we can't define the existence of a given vApp template beforehand
 	/*
-		check.Assert(vcd.vdc.Vdc.ResourceEntities[0].ResourceEntity[0].Name, Equals, "vAppTemplate")
+		check.Assert(vcd.vdc.Vdc.ResourceEntities[0].ResourceEntity[0].Name, Equals, QTVappTemplate)


Looks like "QT" should be "Qt" (or whatever prefix we end up with).

lvirbalas · 2020-05-12T11:38:20Z

CHANGELOG.md

+* Changed signature for `FindAdminCatalogRecords`, which now returns normalized type `[]*types.CatalogRecord`
+* Added methods `catalog.QueryVappTemplateList`, `catalog.QueryCatalogItemList`, `client.queryWithMetadataFields`, `client.queryByMetadataFilter`
+* Added query engine based on `client.SearchByFilter`, type `FilterDef`, and interface `QueryItem`
+* Added methods `adminOrg.QueryCatalogList` and `org.QueryCatalogList`


Just noticed that URL links to the PR are missing. They are esp. important here given the size and complexity of the PR.

lvirbalas

One nit in CHANGELOG.md, otherwise LGTM.

vbauzys

LGTM

Giuseppe Maxia added 4 commits April 3, 2020 07:52

Add queries for catalog item and vapp template

6d833dd

Add queries for vappTemplate/catalogItem metadata

d24e6b8

Merge branch 'master' into query-enhancement

ccbbdf4

format code

f9ae8b8

vmwclabot added the cla-not-required label Apr 3, 2020

Giuseppe Maxia added 5 commits April 10, 2020 15:43

Add query engine

7b237cc

New query engine based on client.SearchByFilter, type FilterDef, and interface QueryItem.

Improve query methods to support filter engine

1573e09

Merge branch 'master' into query-enhancement

ffb0a8c

update CHANGELOG

0c9eae6

Format code

9033dfd

dataclouder changed the title ~~Query enhancement for vApp template and catalog item~~ Query enhancement and filter engine for metadata search Apr 10, 2020

Giuseppe Maxia added 19 commits April 13, 2020 22:09

Improve query engine

765d7ae

* Change SearchByFilter function to use dependency injection * Add unit test for query engine * Add filter type 'earliest' * Add human readable explanation of engine actions

format filter_engine_unit_test.go for easier reading

04ad4a4

add search engine tests

c08158b

update static check

7f50799

expand query search tests

533a241

rename query_search_test.go to filter_engine_test.go

ef889ed

Complete tests for filter engine

007d8cc

Add filter_helpers.go, containing functions used to create filters

Improve filter engine and fix bugs

79436ed

Add query for catalog as org user

be513ee

Add cumulative query retrieval

770016e

Queries that are paginated will iterate until all elements are collected.

Add parent condition to filter interface

01f4371

Improve metadata retrieval

3721926

Add filed type to metadata contents

Add engine search calls for catalog, VDC, Org

1dca1d6

Fix bug in metadata type retrieval

31f8e87

revert metadata type definition

5b6dd91

If we use `xml:"type"`, we get the metadata info, but then the metadata update will fail. We keep it as it is for the time being

improve filter test helpers

5a2f660

* Add more error information * Add guessing of metadata type when API doesn't return the info

Merge branch 'master' into query-enhancement

d8adbf8

update search engine unit test

3e02651

Merge branch 'master' into query-enhancement

fae2873

dataclouder marked this pull request as ready for review April 30, 2020 11:45

dataclouder requested review from lvirbalas, vbauzys and Didainius April 30, 2020 11:53

improve comments for filter_helpers.go

761f1fc

Didainius reviewed May 5, 2020

View reviewed changes

vbauzys reviewed May 5, 2020

View reviewed changes

Giuseppe Maxia added 8 commits May 5, 2020 19:51

Fix bug in catalog items collection

efdc436

When the catalog items are more than the default page size, we don't get all of them. Instead of calling the regular QueryWithNotEncodedParams, we need to run cumulativeQuery

Minor changes

fb2a507

* Improve functions comments * Add to CHANGELOG.md * Make some functions private * Reduce wide scope variable in the search engine

Add inspection info to coding guidelines

1400f27

Add inspection info to search engine

7198a2b

add explanation for SYSTEM metadata

10516eb

add function org.QueryCatalogList

a686d26

Add test for org/adminOrg.QueryCatalogList

780b6bd

Merge branch 'master' into query-enhancement

84a2c7d

lvirbalas reviewed May 12, 2020

View reviewed changes

Giuseppe Maxia added 4 commits May 12, 2020 13:54

update CHANGELOG.md with PR URL

6ec3ddd

Move query engine constants to types/constants.go

ed648bf

update code formatting

72078e0

Fix incorrect query build for metadata search

23b85ca

lvirbalas approved these changes May 12, 2020

View reviewed changes

Didainius approved these changes May 12, 2020

View reviewed changes

Merge branch 'master' into query-enhancement

0a8ec2c

vbauzys approved these changes May 13, 2020

View reviewed changes

dataclouder merged commit 980d170 into vmware:master May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query enhancement and filter engine for metadata search #298

Query enhancement and filter engine for metadata search #298

dataclouder commented Apr 3, 2020

Didainius left a comment

Didainius May 5, 2020

dataclouder May 5, 2020

Didainius May 6, 2020

dataclouder May 6, 2020

vbauzys left a comment

vbauzys May 5, 2020

dataclouder May 5, 2020

vbauzys May 5, 2020

dataclouder May 5, 2020 •

edited

Loading

lvirbalas May 12, 2020

lvirbalas May 12, 2020

lvirbalas left a comment

vbauzys left a comment

Query enhancement and filter engine for metadata search #298

Query enhancement and filter engine for metadata search #298

Conversation

dataclouder commented Apr 3, 2020

Didainius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbauzys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

vbauzys left a comment

Choose a reason for hiding this comment

dataclouder May 5, 2020 •

edited

Loading