Anton Vinogradov created IGNITE-28966:
-----------------------------------------
Summary: 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
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)
