[PR] Reject an unknown runtime-rule catalog instead of classifying it as LAL [skywalking]

["wu-sheng (via GitHub)" <gi...@apache.org>]

wu-sheng opened a new pull request, #13974:
URL: https://github.com/apache/skywalking/pull/13974

   ### Fix an unknown runtime-rule catalog being classified as LAL
   
   `/addOrUpdate` chose its delta classifier with a two-way ternary:
   
   ```java
   delta = DSLScriptKey.isMalCatalog(dslManager.getEngineRegistry(), catalog)
         ? DeltaClassifier.classifyMal(priorContent, content)
         : DeltaClassifier.classifyLal(priorContent, content);
   ```
   
   That was correct when written — #13851 introduced it when MAL and LAL were the only two
   `RuleEngine` implementations, so `!isMal` really did mean LAL. The comment beside it shows the
   author guarding the axis that *could* vary, and doing it well:
   
   > *"routes via RuleEngineRegistry so a catalog declared on `MalRuleEngine.supportedCatalogs`
   > (e.g., telegraf-rules) classifies as MAL automatically, no parallel string list to maintain."*
   
   What could not vary then was the set of engines. The `else` therefore quietly changed meaning from
   "LAL" to "anything that is not MAL":
   
   ```java
   public static boolean isMalCatalog(final RuleEngineRegistry registry, final String catalog) {
       final RuleEngine<?> engine = registry.forCatalog(catalog);   // null when unclaimed
       return engine instanceof MalRuleEngine;                      // null instanceof X == false
   }
   ```
   
   `RuleEngineRegistry.forCatalog` is `byCatalog.get(catalog)`, so an unregistered catalog yields
   `null`, fails the `instanceof`, and lands on the LAL branch. The content is then parsed by the LAL
   classifier and the operator gets **a LAL syntax error for a catalog that does not exist** — or, if
   the content happens to be valid LAL, it proceeds.
   
   #### The change
   
   The decision is now three-valued and each engine has a named branch:
   
   ```java
   static EngineKind engineKindFor(final RuleEngineRegistry registry, final String catalog) {
       final RuleEngine<?> engine = registry.forCatalog(catalog);
       if (engine instanceof MalRuleEngine) { return EngineKind.MAL; }
       if (engine instanceof LalRuleEngine) { return EngineKind.LAL; }
       return EngineKind.NONE;
   }
   ```
   
   `NONE` returns `400 unknown_catalog`. It is extracted to a package-private method rather than left
   inline because the private handler it lives in needs a `DSLManager`, storage and locks to
   construct — which is why this decision had no test.
   
   `DSLScriptKey.isMalCatalog` and `isLalCatalog` are removed with it: the first had exactly one
   caller (this one), the second had none.
   
   #### Behaviour change
   
   **Deliberate and user-visible.** A request naming an unregistered catalog now returns
   `400 unknown_catalog` instead of `400 compile_failed` carrying a misleading LAL parse error. No
   other response changes; the MAL and LAL paths are byte-identical to before.
   
   #### Tests
   
   `RuntimeRuleServiceEngineKindTest`, 4 cases — none of this was covered:
   
   - every catalog `MalRuleEngine` declares resolves to `MAL`, not just the first (the property the
     registry lookup exists to preserve as catalogs are added)
   - `lal` resolves to `LAL`
   - an unregistered catalog, and the empty string, resolve to `NONE` — **the case that previously
     returned LAL**
   - a catalog whose engine has not registered yet resolves to `NONE` rather than falling through to
     whichever branch happens to be last
   
   Mutation-checked: restoring the old "everything else is LAL" shape fails two of them with
   `expected: <NONE> but was: <LAL>`.
   
   - [x] Add a unit test to verify that the fix works.
   - [x] Explain briefly why the bug exists and how to fix it.
   - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
   - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Move DSL class loading under core/dsl, de-duplicate class definition, and name the classify branches [skywalking]

["wu-sheng (via GitHub)" <gi...@apache.org>]

wu-sheng merged PR #13974:
URL: https://github.com/apache/skywalking/pull/13974


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org