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

Add JSON serialization of filters for building more complex queries #113

Merged
merged 19 commits into from
Dec 21, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Dec 3, 2023

Primary change

The URL query parameter spelling is designed to be convenient for simple cases, but it gets awkward with complex cases.

This PR proposes a JSON payload format and a utility to convert that into a runtime filter.

There are loads of ways this could be spelt (and many contrasting examples out there from other projects), with various compromises... the one I picked in this proposal was mainly for ease of implementation with our existing filtering code.

Some of the most significant choices/characteristics:

  • Operators are the parent, values and modifiers are the children:
    • ... so tag != 'abc' is spelt equal: [{not: true, field: "tag", value: "abc"}]
  • Field names are always in a field property - never a key name
  • Operator -> array of matches semantics (makes it very easy to parse in Go strong typing)
  • AND combination is the standard for all operator arrays, and when multiple operators used
  • OR has to be specified explicitly, and the containing entries are all implicit AND (although singulars are optimized out)

Minor other proposals in PR

  • Reducing debug logging to one line per SQL execution
    • Apart from in the case of CRUD.GetMany() where we still have two, but the 2nd includes the count returned
  • Relaxing the requirement on the eventstreams package to require IDs to be UUIDs
    • This matches then the CRUD package

Example (from test)

{
	"skip": 5,
	"limit": 10,
	"sort": [
		"tag",
		"-sequence"
	],
	"equal": [
		{
			"field": "tag",
			"value": "a"
		}
	],
	"greaterThan": [
		{
			"field": "sequence",
			"value": 10
		}
	],
	"or": [
		{
			"equal": [
				{
					"field": "masked",
					"value": true
				}
			],
			"in": [
				{
					"field": "tag",
					"values": [
						"a",
						"b",
						"c"
					]
				}
			]
		},
		{
			"equal": [
				{
					"field": "masked",
					"value": false
				}
			]
		}
	]
}

The AND/OR this results in

(
  tag == 'a'
)
AND
(
  sequence >> 10
)
AND
(
  (
    (
      masked == true
    )
    AND
    (
      tag IN ['a','b','c']
    )
  )
  OR
  (
    masked == false
  )
)
sort=tag,-sequence skip=5 limit=10

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (88b4240) 99.98% compared to head (fa4b399) 99.98%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #113    +/-   ##
========================================
  Coverage   99.98%   99.98%            
========================================
  Files          77       78     +1     
  Lines        6170     6370   +200     
========================================
+ Hits         6169     6369   +200     
  Misses          1        1            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LessThanOrEqual []*FilterJSONKeyValue `ffstruct:"FilterJSON" json:"lessThanOrEqual,omitempty"`
GreaterThan []*FilterJSONKeyValue `ffstruct:"FilterJSON" json:"greaterThan,omitempty"`
GreaterThanOrEqual []*FilterJSONKeyValue `ffstruct:"FilterJSON" json:"greaterThanOrEqual,omitempty"`
In []*FilterJSONKeyValues `ffstruct:"FilterJSON" json:"in,omitempty"`
Copy link
Contributor

@awrichar awrichar Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the two biggest spelling discussions that could come out of this:

  • do we want to spell these out or use abbreviations (many of these JSON query languages use "eq", "gt", etc)?
  • do we want to provide a shortcut semantic for "not", such as prepending "n" or "!" or "not" to the operation name?

There isn't a ton of significance to it, because this is completely functional. But it's something we'll be using for a while.

Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added shortcuts:

  • eq
  • neq
  • lt
  • nlt
  • gt
  • ngt
  • lte
  • nlte
  • gte
  • ngte
  • nin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing these written out, these seem redundant: nlt ngt nlte ngte (since they can all be more logically spelled as gte lte gt lt)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But neq and nin seem useful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the ngts...

pkg/ffapi/filter.go Outdated Show resolved Hide resolved
@awrichar
Copy link
Contributor

awrichar commented Dec 6, 2023

Might be worth some wider community input on the naming of the JSON filters... but overall this seems like a good direction.

@peterbroadhurst
Copy link
Contributor Author

I've made some changes per the very useful feedback @awrichar, and this has soaked for a bit.
My suggestion is we should merge after re-review from @awrichar

Copy link
Contributor

@awrichar awrichar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a few minor comments

@peterbroadhurst peterbroadhurst merged commit fa739e9 into main Dec 21, 2023
2 checks passed
@peterbroadhurst peterbroadhurst deleted the filter-post branch December 21, 2023 19:26
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