[
https://issues.apache.org/jira/browse/IGNITE-28966?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Aleksandr Chesnokov reassigned IGNITE-28966:
--------------------------------------------
Assignee: Aleksandr Chesnokov
> 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
>
> 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)
