Skip to content

fix(file-service, frontend): reject slash in version names - #8883

Merged
kunwp1 merged 3 commits into
apache:mainfrom
kunwp1:fix/dataset-version-slash
Oct 10, 2026
Merged

kunwp1 merged 3 commits into
apache:mainfrom
kunwp1:fix/dataset-version-slash

Conversation

@kunwp1

@kunwp1 kunwp1 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Root cause. createDatasetVersion names a version v{N} - {description} from whatever the user typed, and the UI accepted a /. The version name is one segment of the logical path /<prefix>/owner/resource/version/file, and FileResolver.parsePrefixedPath splits that path on /. For the path in the issue, /texera/bughunt-version-name/v3 - //file_name.png, the version becomes v3 - and /file_name.png becomes the file path, so the version lookup misses and presign-download returns a 500. createModelVersion builds its name the same way, so model versions have the same problem.

Fix. ResourceNaming.validateVersionDescription rejects a description containing / with a 400. createDatasetVersion and createModelVersion call it right after the write-access check, before the LakeFS commit, so the staged files stay staged and the user can fix the description and retry. The version uploader, which datasets and models both use, shows the error under the input and disables Submit, and Enter no longer submits either.

Before:  description "/"  ->  version "v3 - /"  ->  every file in it fails with presign-download 500
After:   description "/"  ->  400 "Invalid version description"  (UI: inline error, Submit disabled)

Not included. Versions already stored with a / in their name stay unopenable, and repairing them needs a data migration. A % or + in a name has a separate problem, because resolveVersionedFile URL-decodes filePath a second time.

Any related issues, documentation, discussions?

Closes #8882

How was this PR tested?

Tests were added for the validator, for both resources, and for the version uploader, and they were written first and failed against the old code.

sbt "FileService/testOnly org.apache.texera.service.resource.ResourceNamingSpec org.apache.texera.service.resource.DatasetResourceSpec org.apache.texera.service.resource.ModelUploadResourceSpec"
cd frontend && yarn ng test --watch=false --include='**/version-uploader.component.spec.ts'

The first command passes 177 tests and the second passes 73. scalafixAll, scalafmtAll, prettier-eslint and eslint ran clean on the changed files.

Screen.Recording.2026-10-06.at.11.49.20.AM.mov

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code, Claude Sonnet 5.5

createDatasetVersion and createModelVersion build the version name
"v{N} - {description}" from free text. The name is one segment of the
logical path /<prefix>/owner/resource/version/file, which FileResolver
splits on "/", so a "/" in the description shifted every later segment
and none of that version's files could be opened again
(presign-download returned a 500).

Reject a description containing "/" with a 400 before the LakeFS commit,
so the staged files stay staged and the user can fix the description and
retry. The version uploader, which datasets and models both use, now
shows the error and disables Submit instead of sending the request.

Versions already stored with a "/" in their name are not repaired.

Closes apache#8882
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @tanishqgandhi1908
    You can notify them by mentioning @tanishqgandhi1908 in a comment.

@Yicong-Huang Yicong-Huang added release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels Oct 6, 2026
@github-actions
github-actions Bot requested review from mengw15 and xuang7 October 6, 2026 18:02
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch Analysis
✅ release/v1.3 Already labeled — this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.
✅ release/v1.2 Already labeled — this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.

Auto-label run.

@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI platform Non-amber Scala service paths labels Oct 6, 2026
@codecov-commenter

codecov-commenter commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.59%. Comparing base (ea0fc10) to head (5e967fb).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8883   +/-   ##
=========================================
  Coverage     92.59%   92.59%           
  Complexity     5059     5059           
=========================================
  Files          1255     1255           
  Lines         53622    53630    +8     
  Branches       6693     6694    +1     
=========================================
+ Hits          49652    49660    +8     
  Misses         2314     2314           
  Partials       1656     1656           
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (ø)
agent-service 99.16% <ø> (ø) Carriedforward from ea0fc10
amber 88.09% <ø> (ø) Carriedforward from ea0fc10
computing-unit-managing-service 60.48% <ø> (ø)
config-service 87.37% <ø> (ø)
file-service 82.00% <100.00%> (+0.05%) ⬆️
frontend 96.57% <100.00%> (+<0.01%) ⬆️
notebook-migration-service 83.73% <ø> (ø)
pyamber 98.58% <ø> (ø) Carriedforward from ea0fc10
workflow-compiling-service 74.09% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kunwp1
kunwp1 marked this pull request as ready for review October 10, 2026 21:27
@mengw15
mengw15 requested a balanced review from Copilot October 10, 2026 21:45
@mengw15 mengw15 removed release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3 labels Oct 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new validation error is inaccessible to screen-reader users, and required manual UI verification details are missing.

2 open findings
What changed in this PR

Prevents invalid dataset and model version descriptions containing /, avoiding unresolvable file paths.

Changes:

  • Adds shared backend validation before LakeFS commits.
  • Adds inline frontend validation and submission guards.
  • Adds backend and frontend regression tests.
File Description
version-uploader.component.ts Blocks invalid submissions.
version-uploader.component.html Displays validation and disables Submit.
version-uploader.component.scss Styles the validation message.
version-uploader.component.spec.ts Tests slash validation and Enter handling.
ResourceNaming.scala Adds shared description validation.
DatasetResource.scala Validates dataset versions before commit.
ModelResource.scala Validates model versions before commit.
ResourceNamingSpec.scala Tests validator boundaries.
DatasetResourceSpec.scala Tests dataset rejection and retry.
ModelUploadResourceSpec.scala Tests model rejection and retry.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mengw15 mengw15 added release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3 labels Oct 10, 2026

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mengw15 mengw15 removed release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3 labels Oct 10, 2026
…ive tech

The slash error was a plain div, so a screen-reader user met a disabled Submit
with no stated reason. Mark the message `role="alert"`, and point the input at
it with `aria-describedby` and `aria-invalid` while the error is showing.

`versionName` is a non-null string, so drop the dead null branch in
`versionNameHasSlash`.
…-slash

# Conflicts:
#	file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala
@kunwp1
kunwp1 enabled auto-merge October 10, 2026 22:24
@kunwp1
kunwp1 added this pull request to the merge queue Oct 10, 2026
Merged via the queue into apache:main with commit adc6a4a Oct 10, 2026
33 checks passed
@kunwp1
kunwp1 deleted the fix/dataset-version-slash branch October 10, 2026 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Files in a dataset version cannot be opened when the version description contains a slash

5 participants