Support multiple listSize directives#8872
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 701404ba0160469d2f37c2b1 |
This comment has been minimized.
This comment has been minimized.
cb41a35 to
db98ca2
Compare
db98ca2 to
1d03895
Compare
1d03895 to
f90457e
Compare
77690d1 to
ab9ef32
Compare
311110b to
9eb0202
Compare
| parsed_sized_fields: Option<Arc<SizedFields>>, | ||
| list_size_directives: Option<Vec<ListSizeDirective>>, | ||
| /// One entry per list_size_directive; parsed at schema load so invalid sizedFields fail startup. | ||
| parsed_sized_fields_per_directive: Vec<Option<Arc<SizedFields>>>, |
There was a problem hiding this comment.
Same question RE having an Option - could this just be Vec<Arc> instead?
| let descended = list_size_directives | ||
| .iter() | ||
| .find_map(|dir| dir.descend(f.name.as_str())) |
There was a problem hiding this comment.
Might be missing the business logic case here - but is there a situation where you'd want this to be a filter_map instead, because there are multiple descended values?
There was a problem hiding this comment.
I looked more into this and you're right. I missed the case where we had 2 leaf nodes at the same level. filter_map is the correct approach. I've updated this section.
1f7ac59 to
926ea73
Compare
|
|
||
| impl SizedFields { | ||
| /// Empty sized fields (no list leaves, no nested paths). Used when a directive has no sizedFields. | ||
| pub(in crate::plugins::demand_control) fn empty() -> Self { |
There was a problem hiding this comment.
Since we removed Option, this means we no longer have the option for a None set of SizedFields when parsing
66e3178 to
1691ce2
Compare
3852e89 to
2863500
Compare
0c8a717 to
2861eaf
Compare
2863500 to
bdda512
Compare
dariuszkuc
left a comment
There was a problem hiding this comment.
The changes look fine for now but once we make the change to repeatable we'll have to clean up the API (e.g. remove list_size_directive_from_field_definition and instead update the code to always process the list)
| sizedFields: [String!] | ||
| requireOneSlicingArgument: Boolean = true | ||
| ) on FIELD_DEFINITION | ||
| ) repeatable on FIELD_DEFINITION |
There was a problem hiding this comment.
this is not a valid schema (as in this federation change still needs to happen) so unsure if this should be included in the PR
Thanks for the review. I agree with the cleanup of the API, which I would slot for the next router release after the federation change. |
2d0604b to
4ac694f
Compare
4ac694f to
921fd8b
Compare
921fd8b to
6800e0e
Compare
This is the final change on the router in the
listSizeenhancements. This PR allows thelistSizedirective to be used multiple times per field. The router aggregates the cost (e.g. takes the max expected_size) so cost stays safe across all usages. Single-directive behavior is unchanged.Why
How (example)
Cost is computed from all directives (e.g. max of their sizes), so limits and budgeting stay correct for every way the field is used.
A federation change will be needed to leverage this feature, which will come at a later date.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
A subsequent PR will contain docs, and the PR to merge the feature branch into
devwill contain the changesetNotes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩