[
https://issues.apache.org/jira/browse/CASSANDRA-20842?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dmitry Konstantinov updated CASSANDRA-20842:
--------------------------------------------
Status: Ready to Commit (was: Review In Progress)
> Fix version range check in MessagingService.getVersionOrdinal
> -------------------------------------------------------------
>
> Key: CASSANDRA-20842
> URL: https://issues.apache.org/jira/browse/CASSANDRA-20842
> Project: Apache Cassandra
> Issue Type: Bug
> Components: Messaging/Internode
> Reporter: Dmitry Konstantinov
> Assignee: Dmitry Konstantinov
> Priority: Normal
> Fix For: 5.x
>
> Attachments: ci_summary_20842-trunk.htm,
> results_details_20842-trunk.tar.xz
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Thank you to [~maedhroz] and Claude :) for highlighting a minor issue in the
> recent changes for CASSANDRA-20816:
> {code:java}
> ## Issues Found
> ### 1. Incorrect bounds check condition
> if (result < 0 || result > maxVersion)
> This condition is wrong. The check should be against the array length, not
> `maxVersion`. Currently:
> - `maxVersion` is the actual version value (e.g., 15)
> - The check should be against `Version.values().length - 1` (the maximum
> valid ordinal)
> ### 2. Misleading variable name
> `maxVersion` actually contains the version value, not the maximum ordinal
> index.
> ## Corrected Implementation
> static final int minVersion = Version.values()[0].value;
> static final int maxVersionOrdinal = Version.values().length - 1;
> /**
> * This is an optimisation to speed up the translation of the serialization
> * version to the {@link Version} enum ordinal.
> * We expect values for new versions to be incremented sequentially
> *
> * @param version the serialization version
> * @return a {@link Version} ordinal value
> */
> public static int getVersionOrdinal(int version)
> {
> int result = version - minVersion;
> if (result < 0 || result > maxVersionOrdinal)
> throw new IllegalStateException("Unknown serialization version: " +
> version);
> return result;
> }
> ## Additional Issues
> ### 3. Typo in exception message
> "Unkown" should be "Unknown"
> ### 4. Test case issue
> The test `checkUnknownBigVersion()` uses `Byte.MAX_VALUE + 1` (128), but this
> assumes version values are byte-sized. A more robust test would use a value
> that's guaranteed to be out of range regardless of the actual version values.
> ## Recommended Test Fix
> @Test(expected = IllegalStateException.class)
> public void checkUnknownBigVersion()
> {
> // Use a value that's definitely larger than any reasonable version
> MessagingService.getVersionOrdinal(1000);
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
