Copilot commented on code in PR #4117:
URL: https://github.com/apache/texera/pull/4117#discussion_r2659288382
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val uid = sessionUser.getUid
+ val dataset = getDatasetByID(ctx, did)
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val document = DocumentFactory
+ .openReadonlyDocument(
+ FileResolver.resolve(s"${getOwner(ctx,
did).getEmail}/${dataset.getName}/$coverImage")
+ )
+ .asInstanceOf[OnDataset]
+
+ val file = LakeFSStorageClient.getFileFromRepo(
+ document.getRepositoryName(),
+ document.getVersionHash(),
+ document.getFileRelativePath()
+ )
+ val coverSizeLimit = 10 * 1024 * 1024 // 10 MB
+
+ if (file.length() > coverSizeLimit) {
+ throw new BadRequestException(
+ s"Cover image must be less than ${coverSizeLimit / (1024 * 1024)} MB"
+ )
+ }
+
+ dataset.setCoverImage(coverImage)
+ new DatasetDao(ctx.configuration()).update(dataset)
+ Response.ok().build()
+ }
+ }
Review Comment:
The new `updateDatasetCoverImage` endpoint lacks test coverage. Since this
repository has test coverage for other dataset endpoints in
DatasetResourceSpec, tests should be added to verify the behavior of this new
endpoint, including: successful cover image update, validation of file size
limits, permission checks for write access, and error handling for invalid file
paths.
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val uid = sessionUser.getUid
+ val dataset = getDatasetByID(ctx, did)
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val document = DocumentFactory
+ .openReadonlyDocument(
+ FileResolver.resolve(s"${getOwner(ctx,
did).getEmail}/${dataset.getName}/$coverImage")
+ )
+ .asInstanceOf[OnDataset]
+
+ val file = LakeFSStorageClient.getFileFromRepo(
+ document.getRepositoryName(),
+ document.getVersionHash(),
+ document.getFileRelativePath()
+ )
Review Comment:
The code does not handle potential exceptions that could be thrown by
`DocumentFactory.openReadonlyDocument()` or
`LakeFSStorageClient.getFileFromRepo()`. If the file path is invalid or the
file doesn't exist, these calls could throw exceptions that aren't properly
caught. Consider adding appropriate error handling to provide more informative
error messages when the specified cover image path is invalid or the file
cannot be accessed.
```suggestion
val (document, file) =
try {
val doc = DocumentFactory
.openReadonlyDocument(
FileResolver.resolve(
s"${getOwner(ctx,
did).getEmail}/${dataset.getName}/$coverImage"
)
)
.asInstanceOf[OnDataset]
val f = LakeFSStorageClient.getFileFromRepo(
doc.getRepositoryName(),
doc.getVersionHash(),
doc.getFileRelativePath()
)
(doc, f)
} catch {
case e: Exception =>
throw new BadRequestException(
s"Unable to access cover image '$coverImage'. Please verify
that the path is valid and the file exists.",
e
)
}
```
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
Review Comment:
The endpoint lacks documentation explaining its purpose, parameters,
expected behavior, and error conditions. Consider adding scaladoc comments to
describe what this endpoint does, what format the coverImage parameter should
be in (e.g., version/relative/path), and what exceptions can be thrown.
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
Review Comment:
The endpoint is missing a `@Consumes` annotation to specify the expected
content type. Based on the frontend code sending a plain string, this should
have `@Consumes(Array(MediaType.TEXT_PLAIN))` to properly document and validate
the API contract. This is inconsistent with other endpoints in this resource
class that explicitly declare their content types.
##########
frontend/src/app/dashboard/service/user/dataset/dataset.service.ts:
##########
@@ -539,4 +539,8 @@ export class DatasetService {
public retrieveOwners(): Observable<string[]> {
return
this.http.get<string[]>(`${AppSettings.getApiEndpoint()}/${DATASET_GET_OWNERS_URL}`);
}
+
+ public updateDatasetCoverImage(did: number, coverImagePath: string):
Observable<Response> {
+ return
this.http.post<Response>(`${AppSettings.getApiEndpoint()}/dataset/${did}/update/cover`,
coverImagePath);
Review Comment:
The HTTP POST body is being sent as a plain string instead of a JSON object.
This is inconsistent with other similar endpoints in this service (like
`updateDatasetName` and `updateDatasetDescription`) which send JSON objects
with properly structured request bodies. Consider wrapping the coverImagePath
in a JSON object for consistency and better API design.
```suggestion
return
this.http.post<Response>(`${AppSettings.getApiEndpoint()}/dataset/${did}/update/cover`,
{
coverImagePath: coverImagePath,
});
```
##########
frontend/src/app/hub/component/browse-section/browse-section.component.ts:
##########
@@ -89,4 +89,12 @@ export class BrowseSectionComponent implements OnInit,
OnChanges {
throw new Error("Unexpected type in DashboardEntry.");
}
}
+
+ getCoverImage(entity: DashboardEntry): string {
+ if (entity.type === "dataset" && entity.coverImageUrl) {
+ const fullPath =
`${entity.ownerEmail}/${entity.name}/${entity.coverImageUrl}`;
Review Comment:
The cover image URL construction does not validate or sanitize the
coverImageUrl value before using it in the path. If coverImageUrl contains
special characters or path traversal sequences (like '../'), this could lead to
unintended file access. While the backend should validate this, the frontend
should also ensure the path is safe before constructing the URL.
```suggestion
/**
* Sanitize a single path segment to prevent path traversal or injection of
* unexpected characters into the constructed file path.
*/
private sanitizePathSegment(segment: string): string {
// Allow only alphanumerics, dot, underscore, and dash; replace others
with "_".
return segment.replace(/[^a-zA-Z0-9._-]/g, "_");
}
getCoverImage(entity: DashboardEntry): string {
if (entity.type === "dataset" && entity.coverImageUrl) {
const owner = this.sanitizePathSegment(entity.ownerEmail);
const name = this.sanitizePathSegment(entity.name);
const cover = this.sanitizePathSegment(entity.coverImageUrl);
const fullPath = `${owner}/${name}/${cover}`;
```
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val uid = sessionUser.getUid
+ val dataset = getDatasetByID(ctx, did)
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val document = DocumentFactory
+ .openReadonlyDocument(
+ FileResolver.resolve(s"${getOwner(ctx,
did).getEmail}/${dataset.getName}/$coverImage")
+ )
+ .asInstanceOf[OnDataset]
+
+ val file = LakeFSStorageClient.getFileFromRepo(
+ document.getRepositoryName(),
+ document.getVersionHash(),
+ document.getFileRelativePath()
+ )
+ val coverSizeLimit = 10 * 1024 * 1024 // 10 MB
Review Comment:
The magic number 10 * 1024 * 1024 for the cover size limit should be
extracted as a named constant at the class or object level. This improves
maintainability and makes it easier to find and update the configuration value.
Consider defining this as a constant like `COVER_IMAGE_SIZE_LIMIT_BYTES` or
moving it to a configuration file.
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1326,4 +1326,44 @@ class DatasetResource {
Right(response)
}
}
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/update/cover")
+ def updateDatasetCoverImage(
+ @PathParam("did") did: Integer,
+ coverImage: String,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val uid = sessionUser.getUid
+ val dataset = getDatasetByID(ctx, did)
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val document = DocumentFactory
+ .openReadonlyDocument(
+ FileResolver.resolve(s"${getOwner(ctx,
did).getEmail}/${dataset.getName}/$coverImage")
+ )
+ .asInstanceOf[OnDataset]
+
+ val file = LakeFSStorageClient.getFileFromRepo(
+ document.getRepositoryName(),
+ document.getVersionHash(),
+ document.getFileRelativePath()
+ )
+ val coverSizeLimit = 10 * 1024 * 1024 // 10 MB
+
+ if (file.length() > coverSizeLimit) {
+ throw new BadRequestException(
+ s"Cover image must be less than ${coverSizeLimit / (1024 * 1024)} MB"
+ )
+ }
Review Comment:
The backend does not validate that the file being set as a cover image is
actually an image file. The endpoint only checks the file size but does not
verify the file type or extension. This could allow non-image files to be set
as cover images, which may cause display issues or security concerns. Consider
adding validation to check that the file extension matches one of the allowed
image types (jpg, jpeg, png, gif, webp) before accepting it as a cover image.
--
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]
