[jira] [Updated] (IGNITE-28966) Protected Classes CI check: classes in the org.apache.ignite.internal package are never detected

["Ignite TC Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/IGNITE-28966?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Ignite TC Bot updated IGNITE-28966:
-----------------------------------
    External issue ID: IGN-2812

> Protected Classes CI check: classes in the org.apache.ignite.internal package are never detected
> ------------------------------------------------------------------------------------------------
>
>                 Key: IGNITE-28966
>                 URL: https://issues.apache.org/jira/browse/IGNITE-28966
>             Project: Ignite
>          Issue Type: Task
>            Reporter: Anton Vinogradov
>            Assignee: Aleksandr Chesnokov
>            Priority: Major
>              Labels: IEP-132, ise
>          Time Spent: 1h
>  Remaining Estimate: 0h
>
> The {{@Order}} annotation sets the number of a field inside a network message. Nodes of different versions read messages by these numbers, so a change here can break a rolling upgrade. The {{Protected Classes}} workflow ({{.github/workflows/check-protected-classes.yml}}) exists to warn a reviewer about such changes.
> The workflow decides that a changed file is protected by searching the whole file text for one string:
> {code:js}
> const ANNOTATION = 'org.apache.ignite.internal.Order';
> ...
> return content.includes(ANNOTATION);
> {code}
> That string is the full class name of the annotation. A class that lives in the {{org.apache.ignite.internal}} package uses {{@Order}} without importing it, so the string never appears in the file and the file is never reported.
> When nothing is found the script stops at {{if (hits.length === 0) return;}}, so the pull request gets no comment, no {{compatibility}} label and no {{Rolling upgrade compatibility}} check. Note that the job {{Rolling Upgrade check}} is green in both cases: it is green even when the check does find something. The real signal for a reviewer is the comment, the label and the separate check. When detection fails none of them appear, so "nothing was found" looks exactly like "nothing was checked". This is the case the job must catch.
> h3. What the check does not see
> 13 message classes, all in {{modules/core/src/main/java/org/apache/ignite/internal/}}: {{ExchangeInfo}}, {{GridJobCancelRequest}}, {{GridJobExecuteRequest}}, {{GridJobExecuteResponse}}, {{GridJobSiblingsRequest}}, {{GridJobSiblingsResponse}}, {{GridTaskCancelRequest}}, {{GridTaskSessionRequest}}, {{GridTopicMessage}}, {{IgniteDiagnosticRequest}}, {{IgniteDiagnosticResponse}}, {{TxEntriesInfo}}, {{TxInfo}}.
> h3. What the check reports by mistake
> The same search fails the other way too, because it looks at the whole file text and not at the import line. 5 files name the class in javadoc, in a string constant, or import it to read it by reflection. None of them has a field with {{@Order(}}, and all of them are reported today:
> * {{modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java}} (javadoc)
> * {{modules/codegen/src/main/java/org/apache/ignite/internal/CustomMapper.java}} (javadoc)
> * {{modules/codegen/src/main/java/org/apache/ignite/internal/MessageProcessor.java}} ({{@SupportedAnnotationTypes}})
> * {{modules/codegen/src/main/java/org/apache/ignite/internal/idto/IDTOSerializerGenerator.java}} ({{el.getAnnotation(Order.class)}})
> * {{modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java}} ({{jarForClass(Order.class)}})
> h3. How to reproduce
> The workflow runs on {{pull_request_target}}. GitHub always takes the workflow file from the base branch, not from the pull request. So a pull request against apache/ignite cannot test its own change to this file: you will still see the old behaviour. Test the change in your own fork. Push it to the fork {{master}}, then open a pull request inside the fork that changes an {{@Order}} number in {{modules/core/src/main/java/org/apache/ignite/internal/GridJobExecuteRequest.java}}.
> Live example: [#13445|https://github.com/apache/ignite/pull/13445] (IGNITE-28270) moves the {{@Order}} numbers of 12 fields in {{GridJobExecuteRequest}} from 13..24 to 14..25. The job {{Rolling Upgrade check}} passes in 5 seconds, and the pull request has no comment and no label.
> h3. Fix
> A file is protected when both rules hold:
> # the file text contains {{@Order(}};
> # the file has the line {{import org.apache.ignite.internal.Order;}} or the line {{package org.apache.ignite.internal;}}.
> Rule 1 alone is not enough. {{org.apache.ignite.internal.systemview.Order}} is a different annotation, imported by 52 system view classes. They are not messages and must stay silent. They are silent today as well, because {{org.apache.ignite.internal.Order}} is not a substring of {{org.apache.ignite.internal.systemview.Order}}.
> Rule 2 alone is not enough: that is today's bug.
> No file in the {{org.apache.ignite.internal}} package imports the system view {{Order}}, so the two rules together are safe. Two details carry weight:
> * The package test must be an exact match on {{package org.apache.ignite.internal;}}. A prefix match brings back the very classes the rule must skip: {{StatisticsColumnConfigurationView}}, {{StatisticsColumnGlobalDataView}}, {{StatisticsColumnLocalDataView}} and {{StatisticsColumnPartitionDataView}} sit in {{org.apache.ignite.internal.processors.query.stat.view}} and use the system view annotation.
> * The open bracket in {{@Order(}} is required. {{Order.java}}, {{MessageMarshallerGenerator.java}} and {{MessageDeploymentGenerator.java}} sit in the same package and write {{@Order}} in javadoc only, never {{@Order(}}.
> New noise: {{modules/core/src/test/resources/codegen/}} holds 72 {{.java}} test data files, and 41 of them match the new rule. The workflow filters on {{path.endsWith('.java')}} only, nothing excludes {{src/test/resources}}, so the new rule will start reporting them. This breaks nothing, but an explicit path exclusion is better.
> Keep the safety rule of this job: it runs on {{pull_request_target}} with a write token, so the pull request files must stay data read through the API. Do not add {{actions/checkout}} of the pull request head, and do not run any pull request code.
> h3. Second defect: an old warning is never cleared
> When nothing is found the script returns before the code that deletes an earlier comment, so a pull request that stopped touching protected classes keeps a stale warning comment.
> The {{compatibility}} label is worse. The script only calls {{addLabels}} and never calls {{removeLabel}}, on any path, so the label is never cleared at all. That part has to be written from scratch, moving the early return is not enough.
> The same block also deletes and re-creates the comment instead of updating it, so every push to a pull request that keeps touching protected classes sends a new notification to everyone subscribed. {{issues.updateComment}} on the existing comment fixes that and the stale comment case at once.
> Expected: when nothing is found, delete the old comment and remove the {{compatibility}} label.
> h3. Third defect: a modified file is read only at the base revision
> {code:js}
> else if (f.status === 'removed' || f.status === 'modified' || f.status === 'changed')
>   revisions.push([f.filename, baseSha]);
> {code}
> A pull request that *adds* {{@Order}} to an existing class, that is, moves the class onto codegen serialization, is never reported, because the base revision has no annotation yet. Unlike the package blindness above, this hole also hides classes outside {{org.apache.ignite.internal}}. A modified file should be read at both revisions, the way {{renamed}} already is.
> h3. Open question
> The {{Rolling upgrade compatibility}} check is created only when something is found. So "checked and clean" looks the same as "the check saw nothing", and the next blind spot will look fine again. Consider creating the check on every run: {{success}} when nothing is found, {{neutral}} when something is found. If this is out of scope, please say so here.
> h3. Acceptance criteria
> * On the current master the new rule matches 566 files: 525 files plus 41 test data files under {{modules/core/src/test/resources/codegen/}}. The old rule matches 518. The difference is +53 (13 message classes and 40 test data files) and -5 (the false positives above).
> * All 13 classes listed above are matched.
> * The 5 files that only name the class in text are not matched.
> * The 52 system view classes stay unmatched.
> * A pull request that changes an {{@Order}} number in {{GridJobExecuteRequest}} gets the comment, the {{compatibility}} label and the {{Rolling upgrade compatibility}} check.
> * A pull request that adds {{@Order}} to an existing class is reported too.
> * A pull request that stopped touching protected classes loses the old comment and the {{compatibility}} label.
> * The workflow still does not check out and does not run the pull request code.
> h3. History and related work
> The script was rewritten from shell and {{grep}} to {{actions/github-script}} in IGNITE-28913. The {{org.apache.ignite.internal.Order}} literal goes back to the first version in IGNITE-28018 and survived IGNITE-28822 and IGNITE-28821 unchanged.
> IGNITE-28716 is Resolved as Implemented, but its pull request [#13171|https://github.com/apache/ignite/pull/13171] is still open (created 2026-05-22, never merged), and none of the files it lists are in master: {{scripts/check-protected-classes.sh}}, {{tests/check-protected-classes.bats}}, {{.github/workflows/test-workflow-scripts.yml}}, {{run-local-check.sh}}. That work moves the same detection into a shell script with BATS tests and a local runner, which is the local check this task needs. But it keeps the same {{org.apache.ignite.internal.Order}} literal, so it does not fix this defect, and it is written against the shape of the workflow before IGNITE-28822 and IGNITE-28913. Before you start, ask on the dev list whether #13171 will be merged. If yes, add the new rule and its test there. If no, fix the inline script in {{check-protected-classes.yml}}.
> IGNITE-25489 asks for a wider check over all {{Message}} subclasses. This ticket only fixes the detection rule of the existing check.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)