AlinsRan commented on PR #13840: URL: https://github.com/apache/apisix/pull/13840#issuecomment-5352292990
Checked all nine against the code. Seven were real and are fixed in `d8804a5b`; one is a deliberate part of the cost model whose comment was wrong; one is not reachable. **Negative quantifiers — real, and the worst of them.** Measured: a query whose honest cost is 2002002 was charged **1** by writing `first: -1` on the outer field. The negative multiplier flips the sign of everything below and the floor of 1 then charges the minimum, so the floor was the bypass rather than the protection. A quantifier is a count, so it now has to be finite and non-negative; anything else is treated as absent, which charges the structure without the fan-out. Zero stays a real count — the upstream resolves nothing under it. **Fragments under a deep index — real.** Reproduced with exactly the case @membphis gave: `Product.expensive` costs 102 alone and 3 as soon as any type-level or three-segment rule elsewhere on the service flips the index to the trie walk. The queue is now reseeded from the fragment's concrete type while the inherited pinned candidates are kept, so a deep path reaching past the fragment still applies. Both fragment forms. Regression asserts 102 under a flat index, under an unrelated three-segment rule and under an unrelated type-level rule, plus a case where a three-segment rule reaching past the fragment does apply — so the test fails if the reseed drops the inherited candidates instead of adding to them. **Shared `introspection_endpoint` cache key — real.** The service now wins as the key and the endpoint is only a fallback. The regression makes both requests inside one block, because Test::Nginx starts a fresh server per block and two separate blocks could never observe the leak; with the old key order the credential-less service is answered `200` from the other's cached schema. **`ofType` depth — real.** Four links; `[[Product!]!]!` is five. Now seven, as a standard introspection query asks. Test asserts the query's expressible depth and that a chain truncated to it still resolves. **`introspection_headers` not encrypted — real,** and slightly worse than stated: this plugin never declared `encrypt_fields` at all beyond what came along with `limit-count`'s schema deepcopy. Appended. **`node_quantifier` fallback — the documentation was wrong, not the behaviour.** That strategy charges only nodes carrying a quantifier, so with no decorations there is nothing to charge and the floor gives 1. Falling back to the complexity walk would make the strategy mean something different depending on whether a decoration exists. Docs now say what each strategy does with no decorations. **`+ 0.01` — deliberate; the comment was wrong.** You are right that `max(..., 1)` already provides the minimum charge and that this rounds every integral cost up by one. It is part of the cost model's definition rather than a floor, and every documented example's published cost includes it, so dropping it means restating them all. The comment claimed to be the thing that makes an undecorated query cost 1; it now says what it actually does. Happy to remove it if you would rather have the arithmetic clean than the published numbers stable — that is a product call, not a correctness one. **Subscription root type — not reachable.** The `graphql` 0.0.2 parser has no `subscription` production: such a document fails `parse` and the plugin answers 400 before the cost engine is entered. It is listed under the parse gaps in the description. Adding `subscriptionType` to the index would be dead code until the parser gains it. **Decoration id colliding with a Service id — not reachable.** The runtime `value.id` of a decoration is not its own id: `config_etcd` overwrites it with the key relative to the prefix, so it reads `aaa-owner/graphql_cost_decorations/hc-svc`. A Service id cannot contain a slash (`id_schema` is `^[a-zA-Z0-9-_.]+$`), so no decoration can ever match a Service id in that lookup. I first added the filter, then reverted it — a guard against something that cannot happen, with a comment saying it can, is worse than none. There is a regression pinning the invariant, since it depends on how `config_etcd` derives the id rather than on anything local to that function. -- 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]
