aicam commented on PR #7763:
URL: https://github.com/apache/texera/pull/7763#issuecomment-5362548756
The move itself is faithful — I diffed both bodies against `main` and
they're byte-identical apart from `private def put` → `def put`, the dropped
`HttpURLConnection`/`URL` imports are genuinely unused now, and all call sites
resolve. My question is about the destination rather than the extraction.
Do these two helpers need a new file? They don't have much in common with
each other — one is an HTTP byte-pusher, the other a JAX-RS path validator —
and each has an existing home that fits it better.
**`put` → `LakeFSStorageClient`**
`common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/util/LakeFSStorageClient.scala`
already owns the rest of the presigned multipart lifecycle:
- `initiatePresignedMultipartUploads`
- `completePresignedMultipartUploads` — takes `partsList: List[(Int,
String)]`, i.e. (part number, ETag)
- `abortPresignedMultipartUploads`
- plus `getFilePresignedUrl` and `parsePhysicalAddress`
`put` is the missing middle step of that same lifecycle.
`DatasetResource.flush()` calls initiate → `put` → complete, and `put`'s return
value *is* the ETag half of the tuple that `completePresignedMultipartUploads`
consumes. Three of the four steps live in one object; this PR extracts the
fourth into a separate file in a different module. file-service already depends
on workflow-core, and `LakeFSFileDocument` in that same module already does raw
`HttpURLConnection` work against presigned URLs on the download side, so this
isn't foreign to the layer.
The one fair objection is that `put` makes no LakeFS API call — it's a
generic S3 presigned PUT. If strict layering is the concern, a
`private`/package-visible `uploadPart` on `LakeFSStorageClient` still seems
better than a new public util, since nothing outside the multipart flow should
be calling it.
**`validateAndNormalizeFilePathOrThrow` → `service/resource`, alongside
`ResourceNaming`**
This one throws `jakarta.ws.rs.BadRequestException`, so it's a request-layer
concern and shouldn't move to workflow-core — agreed it stays in file-service.
But `service/util/` currently holds only `LakeFSExceptionHandler` and
`StagedFileCleanupJob`, both operational rather than validation.
Meanwhile #7760 adds `service/resource/ResourceNaming.scala` — "Naming rules
shared by every user-owned resource" — whose `validateName(label, name)` has
the same shape (validate a user-supplied string, throw 400) and the same
intended audience (datasets and models). A path validator is the direct
counterpart to a name validator. #7764 currently carries both
`ResourceNaming.scala` and `ResourceUploadUtils.scala`, so as the stack lands
we end up with two files doing "validate user-supplied resource strings, throw
400" in two different packages.
Folding the path validator into `ResourceNaming` would also resolve a loose
end here: `DatasetResourcePathSpec` no longer tests anything in
`service.resource` — every assertion now targets `ResourceUploadUtils` — yet it
keeps the `DatasetResource*` name and the `service.resource` package, so
whoever picks up the model-upload work won't find these tests next to the code.
With the split above, the path tests sit beside the naming tests and `put`'s
coverage lands beside `LakeFSStorageClientSpec`. (Worth noting `put` has no
test coverage at present, only `validateAndNormalizeFilePathOrThrow` does.)
**Separately, a few things worth fixing while this code is being lifted**
These are all pre-existing behavior carried over verbatim, but the diff adds
the lines fresh, widens `put` from `private` to public API, and adds a
docstring that asserts behavior the code doesn't implement:
1. `conn.getHeaderField("ETag").replace("\"", "")` dereferences a header
that S3-compatible gateways can omit on a 200/201. The caller then dies with a
bare NPE mid-multipart-upload instead of a diagnosable "part N returned no
ETag", and `conn.disconnect()` on the next line never runs. Worth an
`Option(...)` guard throwing the same shaped `RuntimeException` as the
HTTP-code check above it.
2. No `try`/`finally` in `put`: `out.close()` and `conn.disconnect()` are
only reached on the success path, so a broken pipe mid-write (or the status
check throwing, or the NPE above) leaves the stream open and the socket in the
keep-alive pool. Bounded, since `flush()` doesn't retry, but cheap to fix here.
3. The new docstring says the helper "Rejects empty paths", but it only
rejects empty *input* — it can still return an empty *result*. Against
commons-io 2.20.0, `FilenameUtils.normalize(p, true)` returns `""` for `"."`,
`"./"`, `"a/.."`, and `"a/./.."`; all four pass the null check and the
`getPrefixLength > 0` check and are returned as `""`. A client POSTing
`filePath=a/..` gets an upload session keyed on an empty path, and the LakeFS
object key degenerates to the dataset root. Either reject an empty
`normalized`, or drop the claim from the docstring.
4. The absolute-path guard is bypassable by leading whitespace: `path.trim`
is used for the emptiness test, but the *untrimmed* string is what gets
normalized. `FilenameUtils.normalize(" /etc/passwd", true)` returns `"
/etc/passwd"` with `getPrefixLength == 0`, so it sails past "Absolute paths not
allowed" and is returned verbatim. Impact today is contained — callers use the
result as a LakeFS object key, not a filesystem path, so it's key pollution
rather than traversal — but this is the function whose whole job is that guard,
and it's about to back a second resource type. Normalizing `path.trim` closes
it.
None of this is blocking; the refactor introduces no regression that I can
find.
--
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]
