Rework parseFacets, disallow 'a as orderdesc: b'

[srh]

A lot of parsing functions are implemented in the following style:

for it.Next() {
    if case1 {
        make sure case1 valid
        ...     
    } else if case2 {
        make sure case2 valid
        ...
    } else if case3 {
        make sure case3 valid
        ...
    } ...
}

This is really difficult to work with because each parsing case needs to handle any other possible parsing state. There's basically n^2 stuff to think about.

It also does a poor job of expressing the intent of what is supposed to be parsed. The reason is, all the code paths end up heading to the same place, and it's hard to tell which behaviors are intentional. For example, was this supposed to be valid: @facets(a as orderasc: b) Probably not. But is @facets() supposed to be treated the same as @facets, by setting AllKeys to true? I have no idea -- the code just spills into that case, but there's nothing to indicate whether the possibility was contemplated by the developers.

It's much better to make parsing functions in a manner such that each point in the function body corresponds to a particular kind of place in the parse tree. It's analogous to what happens when you replace an event loop with a stateful object with a goroutine.

It's also a good idea to make liberal use of helper functions like tryParseItemType and trySkipItemVal, or others as we might create.

It looks like due to the way the code was originally written, a lot of parsing code is using this for it.Next() style. I think whenever we work on an old parsing function which uses that style and has gotten too complicated, we should consider refactoring it like this PR does, depending on the time/effort/correctness trade-off.

---

So anyway,

This PR has one material change to the behavior of parseFacets. It disallows @facets(a as orderasc: b).

But there's also the question: Should @facets() still be treated like @facets? Or should it be treated like an empty list of facets? (My guess is we should change it to act like an empty list of facets, because I don't think users would appreciate having a special case.)

---

This change is 

[srh]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

gql/parser.go, line 1654 at r1 (raw file):

	if _, ok := tryParseItemType(it, itemRightRound); ok {
		// TODO: Pre-existing code did all keys on @facets(), instead of no keys.  Might be a

This TODO should be replaced with a comment, and either facets.AllKeys should be set to true or not.

---

Comments from Reviewable

[pawanrawal]

Looks a good change. Makes the code more understandable and easier to maintain. Got some questions and comments.

---

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

gql/parser.go, line 1654 at r1 (raw file):

Previously, srh (Sam Hughes) wrote…

This TODO should be replaced with a comment, and either facets.AllKeys should be set to true or not.

To me sounds like an error or a mistake by the user. We should return an error.

---

gql/parser.go, line 1616 at r2 (raw file):

	// Now try to consume "as".
	if !trySkipItemVal(it, "as") {
		name1 = collectName(it, name1)

I think collectName doesn't need the val as the second argument. It can just start with it.Item().Val. Maybe add a TODO to check for that.

---

gql/parser.go, line 1706 at r2 (raw file):

		}
		if item, ok := tryParseItemType(it, itemComma); !ok {
			if len(facets.Keys) < 2 {

Why this check?

---

Comments from Reviewable

[srh]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

gql/parser.go, line 1654 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

To me sounds like an error or a mistake by the user. We should return an error.

I think there's the potential that generated queries would produce @facets() when their list of facets is empty. I think users wouldn't want to have to work around the edge case of an error. What they'd want is consistent behavior, with an empty facets document for that edge. Otherwise, they'd have to check if their request will retrieve no facets and remove the @facets part entirely, and also add a special case to their handling of the query result.

---

gql/parser.go, line 1616 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I think collectName doesn't need the val as the second argument. It can just start with it.Item().Val. Maybe add a TODO to check for that.

We pass an expression val+item.Val in one of its other callers.

---

gql/parser.go, line 1706 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why this check?

The function either returns an error (which means there was definitely a parse error), or it returns that it could not parse but you could try parseFilter instead. We know it's not a valid parseFilter after we've seen a comma. I'll add commenting to make this more clear.

---

Comments from Reviewable

[ashwin95r]

Just have one comment.

---

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

gql/parser.go, line 1674 at r2 (raw file):

		{
			if facetItem.varName != "" {
				facetVar[facetItem.facetName] = facetItem.varName

Looks like, this will allow @facets(a as w1, a as w2) and @facets(a as w1, b as w1). Both of them should error out. Though I think they'll be caught later. Would be nice to have some tests covering this.

---

Comments from Reviewable

[srh]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

gql/parser.go, line 1674 at r2 (raw file):

Previously, ashwin95r (Ashwin Ramesh) wrote…

Looks like, this will allow @facets(a as w1, a as w2) and @facets(a as w1, b as w1). Both of them should error out. Though I think they'll be caught later. Would be nice to have some tests covering this.

The former will get caught later, duplicate variables. But the latter just overwrites the map key.

We allow duplicate facet keys for some reason. Since we have that, we can disallow multiple variable assignments in the deduplication logic. Or do we want to assign to both variables?

---

Comments from Reviewable

[ashwin95r]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

gql/parser.go, line 1674 at r2 (raw file):

Previously, srh (Sam Hughes) wrote…

The former will get caught later, duplicate variables. But the latter just overwrites the map key.

We allow duplicate facet keys for some reason. Since we have that, we can disallow multiple variable assignments in the deduplication logic. Or do we want to assign to both variables?

I think we should disallow assigning multiple variables to the same key. I can't think of any case where this would be necessary.

And the deduplication I think is to be nicer in case the user gave the same facet twice by mistake.

---

Comments from Reviewable

[pawanrawal]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

gql/parser.go, line 1654 at r1 (raw file):

Previously, srh (Sam Hughes) wrote…

I think there's the potential that generated queries would produce @facets() when their list of facets is empty. I think users wouldn't want to have to work around the edge case of an error. What they'd want is consistent behavior, with an empty facets document for that edge. Otherwise, they'd have to check if their request will retrieve no facets and remove the @facets part entirely, and also add a special case to their handling of the query result.

Ok, sounds good.

---

gql/parser.go, line 1616 at r2 (raw file):

Previously, srh (Sam Hughes) wrote…

We pass an expression val+item.Val in one of its other callers.

Ok

---

Comments from Reviewable

[srh]

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

gql/parser.go, line 1654 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Ok, sounds good.

If we just change the parsing code, it looks like users will still have to handle the special case of there being no @facets document. It seems we do this sort of thing, removing empty result sets, a lot in general. So I don't know that I want to return an empty @facets document if that's not the sort of thing we usually do.

---

gql/parser.go, line 1674 at r2 (raw file):

Previously, ashwin95r (Ashwin Ramesh) wrote…

I think we should disallow assigning multiple variables to the same key. I can't think of any case where this would be necessary.

And the deduplication I think is to be nicer in case the user gave the same facet twice by mistake.

Okay, assigning multiple variable names for a key is disallowed now.

---

Comments from Reviewable

[srh]

In master as of 83b4d74.

[manishrjain]

How do we retrieve all the facets now, without knowing the keys?

Update: Looks like @facets would achieve that, while @facets() would return empty. That's nice!