refactor: Index field directive

[nasdf]

Relevant issue(s)

Resolves #2926

Description

This PR fixes an issue where the index directive was defined twice. The @index field level directive has been renamed to @indexField.

The fields arg has been renamed to includes and is now a list of objects of type:

type IndexFieldInput {
  name: String
  direction: Ordering
}

The direction argument now sets the default direction of all fields in the includes list.

When the index is used on a field definition and the field is not in the includes list it will be implicitly added.

Tasks

• I made sure the code is well commented, particularly hard-to-understand areas.
• I made sure the repository-held documentation is changed accordingly.
• I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
• I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Covered by existing tests.

Specify the platform(s) on which this was tested:

• MacOS

[AndrewSisley]

question: Why can you not just use the same directive in both places? Do they have different arguments?

EDIT: They look like the have the same args, apart from fields which is missing at the field level - can we please add support for field selection indexes defined at the field level and just have one directive?

[codecov]

Codecov Report

Attention: Patch coverage is 95.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.41%. Comparing base (ccf4d93) to head (ba7b8ac).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/request/graphql/schema/collection.go	92.86%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2994      +/-   ##
===========================================
- Coverage    79.41%   79.41%   -0.00%     
===========================================
  Files          329      329              
  Lines        25120    25133      +13     
===========================================
+ Hits         19947    19957      +10     
- Misses        3758     3762       +4     
+ Partials      1415     1414       -1     

Flag	Coverage Δ
all-tests	79.41% <95.00%> (-<0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/request/graphql/schema/manager.go	98.41% <100.00%> (+0.03%)	⬆️
internal/request/graphql/schema/types/types.go	100.00% <100.00%> (ø)
internal/request/graphql/schema/collection.go	89.22% <92.86%> (-1.19%)	⬇️

... and 10 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccf4d93...ba7b8ac. Read the comment docs.

[nasdf]

question: Why can you not just use the same directive in both places? Do they have different arguments?

EDIT: They look like the have the same args, apart from fields which is missing at the field level - can we please add support for field selection indexes defined at the field level and just have one directive?

The arguments are of different types. The @index directive creates an index from one or more fields whereas @indexField is for a singular field.

[AndrewSisley]

The arguments are of different types. The @index directive creates an index from one or more fields whereas @indexField is for a singular field.

Yes, but can we not make them the same type? fields on a field level directive would implicitly include the field that it is on. This way users only have to worry about a single, consistent directive (and so do we, internally).

[nasdf]

Yes, but can we not make them the same type? fields on a field level directive would implicitly include the field that it is on. This way users only have to worry about a single, consistent directive (and so do we, internally).

The direction cannot be implied the same way so you would have something confusing like this:

@index(fields: [name], direction: ASC, directions: [DESC])

[nasdf]

@AndrewSisley and I have come up with some potential improvements to the index directive.

The directions and fields args will be merged into a newincludes arg. The includes arg is a list of type:

input IndexIncludeArg{
  field: String
  direction: Ordering
}

The direction arg will set the default ordering for all fields. When the directive is used on a field definition, that field will be implicitly added to the includes list with the default direction value.

Example of default direction:

@index(name: "idx", direction: ASC, unique: true, includes: [{field: name}, {field: email, direction: DESC}])

would be equivelent to:

@index(name: "idx", unique: true, includes: [{field: name, direction: ASC}, {field: email, direction: DESC}])

[nasdf]

@AndrewSisley and I have come up with some potential improvements to the index directive.

The directions and fields args will be merged into a newincludes arg. The includes arg is a list of type:

input IndexIncludeArg{
  field: String
  direction: Ordering
}

The direction arg will set the default ordering for all fields. When the directive is used on a field definition, that field will be implicitly added to the includes list with the default direction value.

Example of default direction:

@index(name: "idx", direction: ASC, unique: true, includes: [{field: name}, {field: email, direction: DESC}])

would be equivelent to:

@index(name: "idx", unique: true, includes: [{field: name, direction: ASC}, {field: email, direction: DESC}])

Collective decision was to go forward with this design but also allow defining the field orderings in include explicitly as well as implicitly.

[AndrewSisley]

Production code looks good, but please test the new behaviour before merging.

[AndrewSisley]

todo: Please add a Description, and note the implicit/explicit behaviour on it when attached to a field.

These descriptions are important and get rendered in the docs section and intellisense of GQL clients, and I guess that ORMs will also use them to document bound types.

[nasdf]

done

[AndrewSisley]

todo: (not necessarily in this file, this is just the first test file) Please add some integration tests testing implicit field level directives, the 'default' behaviour of the direction property, the overriding of a default direction by explicitly stating it in a field in includes, the overriding of the field location by explicitly stating a field when the directive is hosted on a field, and composite indexes declared on a field.

[nasdf]

done

[fredcarle]

LGTM! Thanks for taking care of this. I like the improvement to defining composite indexes and their components direction.

[AndrewSisley]

LGTM, thanks Keenan!