AlinsRan commented on PR #13840:
URL: https://github.com/apache/apisix/pull/13840#issuecomment-5326458839
Thanks — all three reproduce, and all three are fixed in `16d8075`. I
reproduced each against the engine before changing anything, so the numbers
below are measured rather than argued.
### [P1] Fragment DAGs — confirmed, now bounded
Reproduced with a chain where each fragment spreads the previous one twice:
| n | document | time | cost |
|---|---|---|---|
| 12 | 448 B | 0.004 s | 4098 |
| 16 | 592 B | 0.021 s | 65538 |
| 20 | 736 B | 0.254 s | 1048578 |
Doubling per level, from a document well under a kilobyte, all spent in the
access phase before any limit applies.
I went with the work limit rather than memoisation, deliberately: the cost
of a subtree depends on the type cursor **and** on the live `field_path`
candidates, so a memo key would have to include both, and for `node_quantifier`
the inherited multiplier as well. That is a lot of subtlety to get right for a
bound that a counter gives unconditionally. `each_field` now decrements an
expansion budget per selection and aborts once it is spent; `query_cost`
returns nil plus a client message and the access phase answers 400.
After: n=16/18/20 are all rejected in ~0.01 s, flat instead of doubling.
Regression is `TEST 41`, which asserts both halves — a 10-level DAG still costs
normally (1026), a 24-level one is rejected and the whole thing runs in under a
second.
### [P1] Fragment type conditions — confirmed, cursor now moves
With `Query.node` returning an interface and the weight on the concrete type:
```
Product.expensive add_value=100
query { node { ... on Product { expensive } } } -> 3 (weight missed)
```
Both fragment forms now move the cursor from `typeCondition`, and the same
query costs 102. A condition naming a type the schema does not have leaves the
cursor where it was, so an unknown or ill-formed condition degrades instead of
losing the cursor entirely. `TEST 42` covers the inline form, the named form,
and the unknown-type case.
### [P1] Variable handling — confirmed, safe default
```
first: 10000 -> 20002
first: $n -> 4 (n = 10000)
```
`resolve_variables` now defaults to `true`. Turning it off is still
available for reproducing an engine that ignores variables, but that is no
longer what an operator gets by not thinking about it. `TEST 43` is the
literal-versus-variable equivalence check for both strategies; `TEST 9` and
`TEST 12`–`14` previously asserted the old default and are re-aimed rather than
deleted, so both sides of the switch stay covered.
### Note on the default change
This is a behaviour change for anyone who had already sized a quota against
the old default: the same traffic now costs more, because the fan-out that was
previously invisible is charged. It is called out in the docs and I think it is
the right default — the alternative is shipping a bypass that any client can
use — but it is worth a reviewer's eye rather than being buried in a diff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
