Initial Filter Processor #1220

AlgoStephenAkiki · 2022-09-13T13:25:39Z

Resolves #1100

Adds dynamic filter processor and initial tests

Summary

Explain the goal of this change and what problem it is solving. Format this cleanly so that it may be used for a commit message, as your changes will be squash-merged.

Test Plan

How did you test these changes? Please provide the exact scenarios you tested in as much detail as possible including commands, output and rationale.

Resolves #1100 Adds dynamic filter processor and initial tests

codecov · 2022-09-13T13:45:20Z

Codecov Report

❗ No coverage uploaded for pull request base (conduit@e8fa891). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             conduit    #1220   +/-   ##
==========================================
  Coverage           ?   62.08%           
==========================================
  Files              ?       69           
  Lines              ?     9383           
  Branches           ?        0           
==========================================
  Hits               ?     5825           
  Misses             ?     3059           
  Partials           ?      499

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Eric-Warehime

Really cool stuff! I left some small comments on various things. I think the big thing for me is adding more tests, and splitting the library functions out and getting individual tests for each of those interfaces/utility functions.

processors/config.go

processors/filterprocessor/filter_processor.go

processors/filterprocessor/filter_processor_test.go

Eric-Warehime · 2022-09-13T22:58:19Z

processors/filterprocessor/filter_processor_test.go

+}
+
+// TestFilterProcessor_Init tests initialization of the filter processor
+func TestFilterProcessor_Init(t *testing.T) {


I think we need more tests here. I don't see all being tested, there's not error tests for example supplying a bad tag, or a field which doesn't have the correct operations defined such as a MicroAlgos field.

I added more unit tests. The test that detects either a bad tag or a field that doesnt have the correct function is located in searcher_test.go

Co-authored-by: Eric Warehime <[email protected]>

processors/filterprocessor/expression/expression.go

processors/filterprocessor/config.go

processors/filterprocessor/fields/filter.go

processors/filterprocessor/fields/searcher.go

shiqizng · 2022-09-15T15:48:53Z

processors/filterprocessor/filter_processor_test.go

+      expression: "` + sampleAddr2.String() + `"
+    - tag: SignedTxnWithAD.SignedTxn.Txn.AssetTransferTxnFields.AssetReceiver
+      expression-type: regex
+      expression: "` + sampleAddr2.String() + `"


what happens if all and any have overlapping tags?

The filters are evaluated in a top down order. Meaning that the first filter that gets encountered gets executed.

That's a feature isn't it? i.e. if you want to match two different accounts:

- any: - tag: SignedTxnWithAD.SignedTxn.Txn.Sender expression-type: const expression: "AAAAAAA..." - tag: SignedTxnWithAD.SignedTxn.Txn.Sender expression-type: const expression: "BBBBBBB..."

shiqizng · 2022-09-15T15:51:45Z

processors/filterprocessor/filter_processor_test.go

+      expression: "` + sampleAddr1.String() + `"
+    - tag: SignedTxnWithAD.SignedTxn.Txn.PaymentTxnFields.Receiver
+      expression-type: exact
+      expression: "` + sampleAddr2.String() + `"


what happens if I duplicate one of the tags? Will I get duplicate blocks in result?

Depends on the operator. If it is "any" then if any of the duplicate tags expressions are true then it passes. If it is all, then all the tag expressions must be true. A use case for the latter would be to have a regex that specifies that an address begins and ends with some characters (although you could do that in one regex if you wanted).

Eric-Warehime

There's a few places that I think we should cover--noted them in the comments. I'm working on fixing the codecov report, but I've used the data uploaded from the most recent commit if you're curious https://app.codecov.io/github/algorand/indexer/commit/0fdcf5035e651fd933fe4321914fea7815afb204

Sorry for being paranoid about tests here, but I think because some of this code has potential to cause panics it's better to be extra careful with testing here.

Eric-Warehime · 2022-09-15T22:58:22Z

processors/filterprocessor/filter_processor.go

+			return fmt.Errorf("filter processor Init(): illegal filter tag formation.  tag length was: %d", len(configMaps))
+		}


We don't have a test for this. I think it's something we should test.

Eric-Warehime · 2022-09-15T22:59:27Z

processors/filterprocessor/filter_processor.go

+		for key, subConfigs := range configMaps {
+
+			if !fields.ValidFieldOperation(key) {
+				return fmt.Errorf("filter processor Init(): filter key was not a valid value: %s", key)


This is apparently uncovered--seems like a good test to assert which operations we support.

Eric-Warehime · 2022-09-15T23:01:18Z

processors/filterprocessor/filter_processor.go

+
+				exp, err := expression.MakeExpression(subConfig.ExpressionType, subConfig.Expression)
+				if err != nil {
+					return fmt.Errorf("filter processor Init(): could not make expression with string %s for filter tag %s - %w", subConfig.Expression, subConfig.FilterTag, err)


Looks like this and the error cases of MakeExpression aren't covered.

I've added a filter processor table test that addresses this but I didn't add regex expression specific tests since it would only fail on mal-formed regex.

winder · 2022-09-16T14:43:52Z

processors/filterprocessor/config.go

+type SubConfig struct {
+	// The tag of the struct to analyze
+	FilterTag string `yaml:"tag"`
+	// The type of expression to search for "const" or "regex"


Looks like const this was renamed. Also, since you have comments anyway, could you add the variable names so that they matches the godoc format?

winder · 2022-09-16T14:57:32Z

processors/filterprocessor/filter_processor_test.go

+    - tag: SignedTxnWithAD.SignedTxn.AuthAddr
+      expression-type: exact
+      expression: "` + sampleAddr1.String() + `"
+    - tag: SignedTxnWithAD.SignedTxn.Txn.AssetTransferTxnFields.AssetSender


I think it's important to use the msgpack path here. Have you looked into how that would be implemented?

For example, this would be txn.asnd.

I can take a look for sure. I think its just different logic in the reflection package.

winder

This definitely needs to specify txn fields by their canonical names rather than the go types. I'm not sure that change needs to block the PR.

Otherwise, LGTM.

Initial Filter Processor

5595bcb

Resolves #1100 Adds dynamic filter processor and initial tests

AlgoStephenAkiki self-assigned this Sep 13, 2022

Nits

048156b

AlgoStephenAkiki force-pushed the 1100-implement-filter-processor branch from 47b2643 to 048156b Compare September 13, 2022 13:27

AlgoStephenAkiki marked this pull request as ready for review September 13, 2022 16:29

AlgoStephenAkiki added the Team Lamprey label Sep 13, 2022

AlgoStephenAkiki requested review from Eric-Warehime, shiqizng and winder September 13, 2022 16:30

chaihoang removed the Team Lamprey label Sep 13, 2022

Eric-Warehime suggested changes Sep 13, 2022

View reviewed changes

AlgoStephenAkiki and others added 8 commits September 14, 2022 09:20

Update processors/filterprocessor/filter_processor.go

26b1467

Co-authored-by: Eric Warehime <[email protected]>

Update processors/filterprocessor/filter_processor_test.go

05b818f

Co-authored-by: Eric Warehime <[email protected]>

Update processors/filterprocessor/filter_processor_test.go

19dbbd5

Co-authored-by: Eric Warehime <[email protected]>

PR comments

dc101ad

Pr comments

b585ad2

PR updates

74743a3

PR updates

2979663

Pr updates

b382bbc

winder reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/expression/expression.go Outdated Show resolved Hide resolved

Eric-Warehime reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/expression/expression.go Outdated Show resolved Hide resolved

PR updates

092adcb

shiqizng reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/config.go Outdated Show resolved Hide resolved

shiqizng reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/fields/filter.go Show resolved Hide resolved

shiqizng reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/fields/searcher.go Outdated Show resolved Hide resolved

shiqizng reviewed Sep 14, 2022

View reviewed changes

processors/filterprocessor/fields/searcher.go Outdated Show resolved Hide resolved

AlgoStephenAkiki added 2 commits September 15, 2022 08:21

PR updates

9ed2a78

PR updates

1f381ba

AlgoStephenAkiki requested a review from Eric-Warehime September 15, 2022 12:24

AlgoStephenAkiki requested review from winder and shiqizng September 15, 2022 12:24

Changed some to any and const to exact

9363ada

shiqizng reviewed Sep 15, 2022

View reviewed changes

Test failure fix

0fdcf50

Eric-Warehime suggested changes Sep 15, 2022

View reviewed changes

winder reviewed Sep 16, 2022

View reviewed changes

winder approved these changes Sep 16, 2022

View reviewed changes

AlgoStephenAkiki added 2 commits September 16, 2022 11:08

Added more unit tests

f2aca36

Config comments

6c18d48

AlgoStephenAkiki requested review from Eric-Warehime and shiqizng September 16, 2022 15:12

Eric-Warehime approved these changes Sep 16, 2022

View reviewed changes

AlgoStephenAkiki merged commit 7446007 into conduit Sep 16, 2022

AlgoStephenAkiki deleted the 1100-implement-filter-processor branch September 16, 2022 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Filter Processor #1220

Initial Filter Processor #1220

AlgoStephenAkiki commented Sep 13, 2022

codecov bot commented Sep 13, 2022 •

edited

Loading

Eric-Warehime left a comment

Eric-Warehime Sep 13, 2022

AlgoStephenAkiki Sep 15, 2022

shiqizng Sep 15, 2022

AlgoStephenAkiki Sep 15, 2022

winder Sep 15, 2022

shiqizng Sep 15, 2022

AlgoStephenAkiki Sep 15, 2022

Eric-Warehime left a comment

Eric-Warehime Sep 15, 2022

Eric-Warehime Sep 15, 2022

Eric-Warehime Sep 15, 2022

AlgoStephenAkiki Sep 16, 2022

winder Sep 16, 2022

winder Sep 16, 2022

AlgoStephenAkiki Sep 16, 2022

winder left a comment

		return fmt.Errorf("filter processor Init(): illegal filter tag formation. tag length was: %d", len(configMaps))
		}

Initial Filter Processor #1220

Initial Filter Processor #1220

Conversation

AlgoStephenAkiki commented Sep 13, 2022

Summary

Test Plan

codecov bot commented Sep 13, 2022 • edited Loading

Codecov Report

Eric-Warehime 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Warehime 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2022 •

edited

Loading