jamesnetherton commented on PR #9117:
URL: https://github.com/apache/camel-quarkus/pull/9117#issuecomment-5566174631
Thanks for the fix - splitting the append/page blobs onto their own names is
the right call, and the header override works as expected since
`BlobConfigurationOptionsProxy.getBlobName()` checks `BlobConstants.BLOB_NAME`
in the headers before falling back to the endpoint configuration.
A few things on the `blobVersions` test:
**1. Version selection isn't scoped to the blob under test**
`AzureStorageBlobTest.java:805-813` picks `currentVersion` / `oldVersion`
from every entry returned by `/blob/versions/list`. That listing is
container-wide: `direct:listBlobVersions` sets neither `blobName` nor `regex`,
so `BlobContainerOperations.listBlobVersions` only applies
`BlobListDetails.setRetrieveVersions(true)`.
This PR makes that more likely to bite, since `test-append` and `test-page`
now live in the same container. If any blob other than `test` has versions at
that point, the descending sort can select a foreign `versionId`, which is then
read back against `blobName=test` and will 404 or return unexpected content.
The resource already emits `name` for each entry, so filtering on it should
be enough:
```java
.filter(v -> BLOB_NAME.equals(v.get("name")))
```
on both streams.
**2. The new comparator can NPE**
`AzureStorageBlobTest.java:812` dereferences `versionId` unconditionally,
but `AzureStorageBlobResource` only adds that key when it is non-null:
```java
if (blobItem.getVersionId() != null) {
versionBuilder.add("versionId", blobItem.getVersionId());
}
versionBuilder.add("isCurrentVersion", blobItem.isCurrentVersion() != null
&& blobItem.isCurrentVersion());
```
`isCurrentVersion` is always present and defaults to `false`, so an entry
with no version id (a blob written before versioning was enabled on the
account, for example) passes the `!isCurrentVersion` filter and then NPEs
inside `compareTo`. That surfaces as a bare NPE rather than a useful assertion
failure. Either filter out null ids first, or use `Comparator.comparing(v ->
(String) v.get("versionId"),
Comparator.nullsFirst(Comparator.naturalOrder())).reversed()`.
**3. README wording**
`BlobVersioningEnabled` also requires
`!MockBackendUtils.startMockBackend()`, and the mock backend is on by default.
The next paragraph does mention `CAMEL_QUARKUS_START_MOCK_BACKEND=false`, but
as "You may want to" - for these tests it is required, otherwise they are
silently skipped. Worth saying so in the new section.
Nothing here breaks the intended flow, and the rest looks good to me - the
descending sort does select v1 for the create/update sequence, so dropping the
`assertEquals(2, versions.size())` assertion is sound.
---
_This review was AI-generated, on behalf of James Netherton._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
