[
https://issues.apache.org/jira/browse/CASSANDRA-20842?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dmitry Konstantinov updated CASSANDRA-20842:
--------------------------------------------
Description:
Thank you to [~maedhroz] and Claude :) to highlight 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}
was:
Thank you to [~maedhroz] and Claude :) to highlight a minor issue in the recent
changes:
{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}
> 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
>
>
> Thank you to [~maedhroz] and Claude :) to highlight 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
