This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8000-646e460838c568dc421042ac8bcd95aa534aa118 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 516087ed04859a4252261330cac8acf265b0a1ee Author: Tanishq Gandhi <[email protected]> AuthorDate: Thu Aug 27 22:58:47 2026 +0000 feat(file-service): add model upload limits and the models_enabled flag (#8000) ### What changes were proposed in this PR? 1. **`gui.tabs.models_enabled`, default `false`** — the flag the model UI will gate on. Nothing reads it yet; it hides the sidebar entry, not the route (same as `datasets_enabled`). 2. **Models get their own upload ceiling: 2 GiB, up from the 20 MiB they inherited from datasets.** The shared upload engine read the dataset's `single_file_upload_max_size_mib` key literally, so both types resolved the same site setting. 3. **An `UploadLimits` descriptor** — ties a resource type to its upload keys and exposes `singleFileUploadMaxBytes`. `ResourceStorage` gains an `uploadLimits` slot, so both call sites read the limit through the descriptor instead of naming a key. The same helper also existed in `DatasetResource` and `ModelResource`, where it was dead code; both copies removed. **Naming (review note).** `site_settings` rows are keyed by a leaf's *last path segment*, so `dataset { }` / `model { }` are flattened away before storage and two bare leaves would collide — `DefaultsConfig`'s uniqueness `require` fails at load. So the model leaves need a prefix, and per review the dataset leaves got one too: ``` dataset { dataset_single_file_upload_max_size_mib = 20 } model { model_single_file_upload_max_size_mib = 2048 } ``` Clean rename, no alias. Also fixes `MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE` -> `DATASET_MAX_...`. That rename is why this touches the frontend and ships a migration: - The key strings are hardcoded in `admin-settings`, `files-uploader` and `dataset-detail`. - `sql/updates/42.sql` renames the four existing `site_settings` rows — otherwise the seeder recreates them at defaults and an admin's raised limit is silently lost. Idempotent. `ConfigResource` derives its public whitelist from `gui`/`dataset`/`model`, which is why the pinned set in `ConfigResourceSpec` changed. ### Any related issues, documentation, discussions? Backend prerequisite for the model dashboard UI — #6499 and #6501. ### How was this PR tested? New `UploadLimitsSpec` (5 tests, no DB harness): pins each descriptor's keys against `default.conf`, asserts the dataset and model key sets are disjoint, and asserts the model ceiling resolves to 2 GiB. `42.sql` run against a scratch Postgres: values preserved, unrelated rows untouched, second run a no-op, no PK violation when both old and new rows exist. Existing suites pass unmodified except the two pinned config assertions: `DefaultsConfigSpec` 5, `ConfigResourceSpec` 33, `UploadLimitsSpec` 5, `DatasetResourceSpec` 152, `ModelUploadResourceSpec` 13, `ModelResourceSpec` 22, `admin-settings` 45, `dataset-detail` 203, `files-uploader` 30. scalafmt + prettier clean. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --- common/config/src/main/resources/default.conf | 41 +++++++--- .../texera/common/config/DefaultsConfigSpec.scala | 26 +++++-- .../texera/service/resource/ConfigResource.scala | 16 ++-- .../service/resource/ConfigResourceSpec.scala | 19 +++-- .../texera/service/resource/DatasetResource.scala | 4 - .../texera/service/resource/ModelResource.scala | 5 +- .../service/resource/ResourceUploadService.scala | 22 +++--- .../texera/service/resource/UploadLimits.scala | 74 ++++++++++++++++++ .../service/resource/DatasetResourceSpec.scala | 2 +- .../texera/service/resource/UploadLimitsSpec.scala | 87 ++++++++++++++++++++++ .../settings/admin-settings.component.spec.ts | 36 ++++----- .../admin/settings/admin-settings.component.ts | 30 +++++--- .../files-uploader/files-uploader.component.ts | 2 +- .../dataset-detail.component.spec.ts | 6 +- .../dataset-detail.component.ts | 6 +- sql/changelog.xml | 5 ++ sql/updates/42.sql | 73 ++++++++++++++++++ 17 files changed, 365 insertions(+), 89 deletions(-) diff --git a/common/config/src/main/resources/default.conf b/common/config/src/main/resources/default.conf index 6b83f50f6b..f46fbf0a18 100644 --- a/common/config/src/main/resources/default.conf +++ b/common/config/src/main/resources/default.conf @@ -64,6 +64,10 @@ gui { datasets_enabled = true datasets_enabled = ${?GUI_TABS_DATASETS_ENABLED} + # Hides the Models sidebar entry, not the route. + models_enabled = false + models_enabled = ${?GUI_TABS_MODELS_ENABLED} + compute_enabled = true compute_enabled = ${?GUI_TABS_COMPUTE_ENABLED} @@ -80,24 +84,41 @@ gui { } dataset { - single_file_upload_max_size_mib = 20 - single_file_upload_max_size_mib = ${?DATASET_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB} + dataset_single_file_upload_max_size_mib = 20 + dataset_single_file_upload_max_size_mib = ${?DATASET_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB} + + dataset_max_number_of_concurrent_uploading_file = 3 + dataset_max_number_of_concurrent_uploading_file = ${?DATASET_MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE} + + # The maximum number of file chunks that can be held in the memory + dataset_max_number_of_concurrent_uploading_file_chunks = 10 + dataset_max_number_of_concurrent_uploading_file_chunks = ${?DATASET_MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE_CHUNKS} + + # the size of each chunk during the multipart upload of file + dataset_multipart_upload_chunk_size_mib = 50 + dataset_multipart_upload_chunk_size_mib = ${?DATASET_MULTIPART_UPLOAD_CHUNK_SIZE_MIB} +} + +model { + model_single_file_upload_max_size_mib = 2048 + model_single_file_upload_max_size_mib = ${?MODEL_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB} - max_number_of_concurrent_uploading_file = 3 - max_number_of_concurrent_uploading_file = ${?MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE} + model_max_number_of_concurrent_uploading_file = 3 + model_max_number_of_concurrent_uploading_file = ${?MODEL_MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE} # The maximum number of file chunks that can be held in the memory - max_number_of_concurrent_uploading_file_chunks = 10 - max_number_of_concurrent_uploading_file_chunks = ${?DATASET_MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE_CHUNKS} + model_max_number_of_concurrent_uploading_file_chunks = 10 + model_max_number_of_concurrent_uploading_file_chunks = ${?MODEL_MAX_NUMBER_OF_CONCURRENT_UPLOADING_FILE_CHUNKS} # the size of each chunk during the multipart upload of file - multipart_upload_chunk_size_mib = 50 - multipart_upload_chunk_size_mib = ${?DATASET_MULTIPART_UPLOAD_CHUNK_SIZE_MIB} + model_multipart_upload_chunk_size_mib = 50 + model_multipart_upload_chunk_size_mib = ${?MODEL_MULTIPART_UPLOAD_CHUNK_SIZE_MIB} } # Operator-level defaults. These are management-only site settings (edited from -# the admin page, read by the engine) and are deliberately NOT under gui/dataset, -# so they stay out of the anonymous /config/settings/public whitelist. +# the admin page, read by the engine) and are deliberately NOT under +# gui/dataset/model, so they stay out of the anonymous /config/settings/public +# whitelist. operator { # Upper bound on the number of columns the CSV scan source will parse. csv_parser_max_columns = 512 diff --git a/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala b/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala index e7bf3fb6ab..e675ebcda2 100644 --- a/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala +++ b/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala @@ -43,9 +43,14 @@ class DefaultsConfigSpec extends AnyFlatSpec with Matchers { defaults should not be empty // scalar leaves are flattened to their last path segment ifUnset("DATASET_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB")( - defaults.get("single_file_upload_max_size_mib") shouldBe Some("20") + defaults.get("dataset_single_file_upload_max_size_mib") shouldBe Some("20") ) ifUnset("GUI_TABS_HUB_ENABLED")(defaults.get("hub_enabled") shouldBe Some("true")) + // the model block's leaves are `model_`-prefixed to keep short keys unique + ifUnset("MODEL_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB")( + defaults.get("model_single_file_upload_max_size_mib") shouldBe Some("2048") + ) + ifUnset("GUI_TABS_MODELS_ENABLED")(defaults.get("models_enabled") shouldBe Some("false")) // management-only keys are flattened too (used by reset + the startup seeder) ifUnset("OPERATOR_CSV_PARSER_MAX_COLUMNS")( defaults.get("csv_parser_max_columns") shouldBe Some("512") @@ -54,13 +59,13 @@ class DefaultsConfigSpec extends AnyFlatSpec with Matchers { defaults.values.foreach(_ shouldBe a[String]) } - it should "keep management-only keys out of the public gui/dataset whitelist" in { + it should "keep management-only keys out of the public gui/dataset/model whitelist" in { // csv_parser_max_columns is seeded and resettable (present in allDefaults) // but lives under `operator`, so it must never reach the anonymous // /config/settings/public payload. DefaultsConfig.allDefaults.keySet should contain("csv_parser_max_columns") DefaultsConfig.keysUnderSections( - Set("gui", "dataset") + Set("gui", "dataset", "model") ) should not contain "csv_parser_max_columns" } @@ -68,17 +73,24 @@ class DefaultsConfigSpec extends AnyFlatSpec with Matchers { val guiKeys = DefaultsConfig.keysUnderSections(Set("gui")) guiKeys should contain allOf ("logo", "mini_logo", "favicon", "hub_enabled") // keys from other sections are excluded - guiKeys should not contain "single_file_upload_max_size_mib" + guiKeys should not contain "dataset_single_file_upload_max_size_mib" guiKeys should not contain "always-reset-configurations-to-default-values" val datasetKeys = DefaultsConfig.keysUnderSections(Set("dataset")) - datasetKeys should contain("single_file_upload_max_size_mib") + datasetKeys should contain("dataset_single_file_upload_max_size_mib") datasetKeys should not contain "logo" + + // `model` is a sibling section, not a sub-section of `dataset`: disjoint sets + // are what let a model carry a different ceiling. + val modelKeys = DefaultsConfig.keysUnderSections(Set("model")) + modelKeys should contain("model_single_file_upload_max_size_mib") + modelKeys should not contain "dataset_single_file_upload_max_size_mib" + datasetKeys should not contain "model_single_file_upload_max_size_mib" } it should "union multiple sections and be empty for an unknown section" in { - val union = DefaultsConfig.keysUnderSections(Set("gui", "dataset")) - union should contain allOf ("logo", "single_file_upload_max_size_mib") + val union = DefaultsConfig.keysUnderSections(Set("gui", "dataset", "model")) + union should contain allOf ("logo", "dataset_single_file_upload_max_size_mib", "model_single_file_upload_max_size_mib") // every returned key exists in allDefaults under the same short name union.subsetOf(DefaultsConfig.allDefaults.keySet) shouldBe true diff --git a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala index 18a35edc0a..23c1e7fa68 100644 --- a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala +++ b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala @@ -117,21 +117,21 @@ class ConfigResource { ) // The site_settings keys that non-admin pages consume: dashboard branding, - // sidebar tab toggles, and dataset upload limits — exactly the gui.* and - // dataset.* sections of default.conf, which is also where the seeding - // pipeline gets them. Keys declared outside those sections (e.g. + // sidebar tab toggles, and the dataset/model upload limits — exactly the + // gui.*, dataset.* and model.* sections of default.conf, which is also where + // the seeding pipeline gets them. Keys declared outside those sections (e.g. // csv_parser_max_columns) are management-only. Deriving the set from the // file keeps "which section does this default live in" the single place // where visibility is decided. private val publicSettingKeys: Set[String] = - DefaultsConfig.keysUnderSections(Set("gui", "dataset")) + DefaultsConfig.keysUnderSections(Set("gui", "dataset", "model")) // SECURITY: every key returned here is served anonymously (see // /settings/public below), so `publicSettingKeys` is the anonymous-exposure - // surface. It is derived from the gui/dataset sections of default.conf and - // pinned by ConfigResourceSpec/DefaultsConfigSpec — adding a key under those - // sections (or moving one in) changes what unauthenticated callers can read - // and MUST be reviewed there. Never place a secret under gui/dataset. + // surface. It is derived from the gui/dataset/model sections of default.conf + // and pinned by ConfigResourceSpec/DefaultsConfigSpec — adding a key under + // those sections (or moving one in) changes what unauthenticated callers can + // read and MUST be reviewed there. Never place a secret under those sections. private def fetchSettings(condition: Condition): Map[String, String] = ctx diff --git a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala index 295de41c56..6c569db80d 100644 --- a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala +++ b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala @@ -401,11 +401,11 @@ class ConfigResourceSpec publicSettings should not contain key("csv_parser_max_columns") } - // The public whitelist is derived from the gui/dataset sections of + // The public whitelist is derived from the gui/dataset/model sections of // default.conf. This pins the derived set, so moving a key between sections // (or adding one) forces the visibility decision into review here. - it should "expose exactly the gui and dataset section keys of default.conf" in { - DefaultsConfig.keysUnderSections(Set("gui", "dataset")) shouldBe Set( + it should "expose exactly the gui, dataset and model section keys of default.conf" in { + DefaultsConfig.keysUnderSections(Set("gui", "dataset", "model")) shouldBe Set( "logo", "mini_logo", "favicon", @@ -417,14 +417,19 @@ class ConfigResourceSpec "projects_enabled", "workflows_enabled", "datasets_enabled", + "models_enabled", "compute_enabled", "quota_enabled", "forum_enabled", "about_enabled", - "single_file_upload_max_size_mib", - "multipart_upload_chunk_size_mib", - "max_number_of_concurrent_uploading_file", - "max_number_of_concurrent_uploading_file_chunks" + "dataset_single_file_upload_max_size_mib", + "dataset_multipart_upload_chunk_size_mib", + "dataset_max_number_of_concurrent_uploading_file", + "dataset_max_number_of_concurrent_uploading_file_chunks", + "model_single_file_upload_max_size_mib", + "model_multipart_upload_chunk_size_mib", + "model_max_number_of_concurrent_uploading_file", + "model_max_number_of_concurrent_uploading_file_chunks" ) } diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala index 6c15196a0b..76eb3ebd74 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala @@ -29,7 +29,6 @@ import org.apache.texera.common.util.EmailUtil import org.apache.texera.amber.core.storage.util.LakeFSStorageClient import org.apache.texera.amber.core.storage.ResourceType import org.apache.texera.auth.SessionUser -import org.apache.texera.dao.SiteSettings import org.apache.texera.dao.SqlServer import org.apache.texera.dao.SqlServer.withTransaction import org.apache.texera.dao.jooq.generated.enums.{PrivilegeEnum, UserRoleEnum} @@ -72,9 +71,6 @@ object DatasetResource { .getInstance() .createDSLContext() - private def singleFileUploadMaxBytes(defaultMiB: Long = 20L): Long = - SiteSettings.getLong("single_file_upload_max_size_mib", defaultMiB) * 1024L * 1024L - /** * Helper function to get the dataset from DB using did */ diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala index 1ef4fddfc7..54cf7cc5ce 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala @@ -28,7 +28,7 @@ import org.apache.texera.amber.core.storage.ResourceType import org.apache.texera.amber.core.storage.util.LakeFSStorageClient import org.apache.texera.auth.SessionUser import org.apache.texera.common.config.StorageConfig -import org.apache.texera.dao.{SiteSettings, SqlServer} +import org.apache.texera.dao.SqlServer import org.apache.texera.dao.SqlServer.withTransaction import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum import org.apache.texera.dao.jooq.generated.tables.Model.MODEL @@ -107,9 +107,6 @@ object ModelResource { .getInstance() .createDSLContext() - private def singleFileUploadMaxBytes(defaultMiB: Long = 20L): Long = - SiteSettings.getLong("single_file_upload_max_size_mib", defaultMiB) * 1024L * 1024L - /** * Helper function to get the model from DB using mid */ diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala index b974cf31a8..f95a0f92f4 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala @@ -26,7 +26,7 @@ import org.apache.texera.amber.core.storage.model.OnVersionedFileResource import org.apache.texera.amber.core.storage.util.LakeFSStorageClient import org.apache.texera.amber.core.storage.{DocumentFactory, FileResolver} import org.apache.texera.common.config.StorageConfig -import org.apache.texera.dao.{SiteSettings, SqlServer} +import org.apache.texera.dao.SqlServer import org.apache.texera.dao.SqlServer.withTransaction import org.apache.texera.dao.jooq.generated.tables.Dataset.DATASET import org.apache.texera.dao.jooq.generated.tables.Model.MODEL @@ -74,11 +74,11 @@ import scala.util.Try * Describes where a resource's files live and how its in-progress uploads are tracked. * * [[ResourceTables]] names the columns that carry identity, ownership and grants; this adds - * the storage side — the LakeFS repository column plus the `*_upload_session` and - * `*_upload_session_part` tables that back resumable multipart uploads. Naming the columns - * keeps one implementation of the upload engine serving every resource type, so adding the - * next one costs a descriptor rather than another copy of the locking, part-size and - * resume rules. + * the storage side — the LakeFS repository column, the [[UploadLimits]] this type is held to, + * and the `*_upload_session` and `*_upload_session_part` tables that back resumable multipart + * uploads. Naming them keeps one implementation of the upload engine serving every resource + * type, so adding the next one costs a descriptor rather than another copy of the locking, + * part-size and resume rules. * * @tparam R record type of the resource table * @tparam A record type of the companion user-access table @@ -88,6 +88,7 @@ import scala.util.Try case class ResourceStorage[R <: Record, A <: Record, S <: Record, P <: Record]( resource: ResourceTables[R, A], resourceType: ResourceType.Value, + uploadLimits: UploadLimits, repositoryNameField: TableField[R, String], sessionResourceId: TableField[S, Integer], sessionUid: TableField[S, Integer], @@ -117,6 +118,7 @@ object ResourceStorage { ResourceStorage( resource = ResourceTables.Dataset, resourceType = ResourceType.Dataset, + uploadLimits = UploadLimits.Dataset, repositoryNameField = DATASET.REPOSITORY_NAME, sessionResourceId = DATASET_UPLOAD_SESSION.DID, sessionUid = DATASET_UPLOAD_SESSION.UID, @@ -141,6 +143,7 @@ object ResourceStorage { ResourceStorage( resource = ResourceTables.Model, resourceType = ResourceType.Model, + uploadLimits = UploadLimits.Model, repositoryNameField = MODEL.REPOSITORY_NAME, sessionResourceId = MODEL_UPLOAD_SESSION.MID, sessionUid = MODEL_UPLOAD_SESSION.UID, @@ -172,9 +175,6 @@ object ResourceUploadService { .getInstance() .createDSLContext() - private def singleFileUploadMaxBytes(defaultMiB: Long = 20L): Long = - SiteSettings.getLong("single_file_upload_max_size_mib", defaultMiB) * 1024L * 1024L - /** * Builds the file nodes of one committed version, plus the version's total size. * @@ -714,7 +714,7 @@ object ResourceUploadService { if (fileSizeBytesValue <= 0L) throw new BadRequestException("fileSizeBytes must be > 0") if (partSizeBytesValue <= 0L) throw new BadRequestException("partSizeBytes must be > 0") - val totalMaxBytes: Long = singleFileUploadMaxBytes() + val totalMaxBytes: Long = s.uploadLimits.singleFileUploadMaxBytes if (totalMaxBytes <= 0L) { throw new WebApplicationException( "singleFileUploadMaxBytes must be > 0", @@ -1105,7 +1105,7 @@ object ResourceUploadService { ) } - val maxBytes = singleFileUploadMaxBytes() + val maxBytes = s.uploadLimits.singleFileUploadMaxBytes val tooLarge = actualSizeBytes > maxBytes if (tooLarge) { diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/UploadLimits.scala b/file-service/src/main/scala/org/apache/texera/service/resource/UploadLimits.scala new file mode 100644 index 0000000000..6c5fa09340 --- /dev/null +++ b/file-service/src/main/scala/org/apache/texera/service/resource/UploadLimits.scala @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import org.apache.texera.dao.SiteSettings + +/** An admin-editable limit: its `site_settings` key, and the fallback when the row is missing + * (must match the leaf's default in `default.conf`). + */ +case class UploadLimit(key: String, defaultValue: Long) + +/** + * The upload ceilings of one resource type. + * + * `site_settings` rows are keyed by a `default.conf` leaf's last path segment, so each type + * needs its own key names. Only the size ceiling is enforced server-side; the chunk-size and + * concurrency limits are client-side tuning served from `/config/settings/public`. + */ +case class UploadLimits( + singleFileMaxSizeMiB: UploadLimit, + multipartChunkSizeMiB: UploadLimit, + maxConcurrentFiles: UploadLimit, + maxConcurrentFileChunks: UploadLimit +) { + + /** The largest single file this resource type accepts, in bytes. */ + def singleFileUploadMaxBytes: Long = + SiteSettings.getLong( + singleFileMaxSizeMiB.key, + singleFileMaxSizeMiB.defaultValue + ) * 1024L * 1024L + + def all: Seq[UploadLimit] = + Seq(singleFileMaxSizeMiB, multipartChunkSizeMiB, maxConcurrentFiles, maxConcurrentFileChunks) +} + +object UploadLimits { + + val Dataset: UploadLimits = + UploadLimits( + singleFileMaxSizeMiB = UploadLimit("dataset_single_file_upload_max_size_mib", 20L), + multipartChunkSizeMiB = UploadLimit("dataset_multipart_upload_chunk_size_mib", 50L), + maxConcurrentFiles = UploadLimit("dataset_max_number_of_concurrent_uploading_file", 3L), + maxConcurrentFileChunks = + UploadLimit("dataset_max_number_of_concurrent_uploading_file_chunks", 10L) + ) + + // Model weights are far larger than the files datasets are sized for: 2 GiB, not 20 MiB. + val Model: UploadLimits = + UploadLimits( + singleFileMaxSizeMiB = UploadLimit("model_single_file_upload_max_size_mib", 2048L), + multipartChunkSizeMiB = UploadLimit("model_multipart_upload_chunk_size_mib", 50L), + maxConcurrentFiles = UploadLimit("model_max_number_of_concurrent_uploading_file", 3L), + maxConcurrentFileChunks = + UploadLimit("model_max_number_of_concurrent_uploading_file_chunks", 10L) + ) +} diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala index e0931570fc..b728b748c5 100644 --- a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala @@ -1654,7 +1654,7 @@ class DatasetResourceSpec s"$prefix/${System.nanoTime()}-${Random.alphanumeric.take(8).mkString}.bin" // ---------- site_settings helpers (max upload size) ---------- - private val MaxUploadKey = "single_file_upload_max_size_mib" + private val MaxUploadKey = "dataset_single_file_upload_max_size_mib" private def upsertSiteSetting(key: String, value: String): Unit = { val table = DSL.table(DSL.name("texera_db", "site_settings")) diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/UploadLimitsSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/UploadLimitsSpec.scala new file mode 100644 index 0000000000..09e9c7a0e9 --- /dev/null +++ b/file-service/src/test/scala/org/apache/texera/service/resource/UploadLimitsSpec.scala @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import org.apache.texera.common.config.DefaultsConfig +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Unit tests for [[UploadLimits]] and its wiring into [[ResourceStorage]]. Pure: `SiteSettings` + * falls back to the caller's default when `SqlServer` is uninitialised, so no DB harness is + * needed. These pin each descriptor's keys against `default.conf` and keep the two types' + * key sets disjoint — sharing one is how model uploads came to be capped at 20 MiB. + */ +class UploadLimitsSpec extends AnyFlatSpec with Matchers { + + // Each leaf carries a ${?ENV} override, which would legitimately move the default. + private def ifUnset(name: String)(assertion: => Any): Unit = + if (!sys.env.contains(name) && !sys.props.contains(name)) assertion + + private def defaultOf(key: String): Option[String] = DefaultsConfig.allDefaults.get(key) + + /** Every limit must name a default.conf leaf AND carry that leaf's value: a descriptor + * default that has drifted from the file is what the fallback silently serves whenever + * the site_settings row is missing. Each leaf's env override is its key uppercased. + */ + private def assertDeclaredDefaults(limits: UploadLimits): Unit = + limits.all.foreach { limit => + withClue(s"${limit.key}: ") { + defaultOf(limit.key) shouldBe defined + ifUnset(limit.key.toUpperCase)( + defaultOf(limit.key) shouldBe Some(limit.defaultValue.toString) + ) + } + } + + "UploadLimits.Dataset" should "name keys that default.conf declares, with matching defaults" in { + assertDeclaredDefaults(UploadLimits.Dataset) + UploadLimits.Dataset.singleFileMaxSizeMiB.defaultValue shouldBe 20L + } + + "UploadLimits.Model" should "name keys that default.conf declares, with matching defaults" in { + assertDeclaredDefaults(UploadLimits.Model) + UploadLimits.Model.singleFileMaxSizeMiB.defaultValue shouldBe 2048L + } + + it should "not share a single key with the dataset limits" in { + val datasetKeys = UploadLimits.Dataset.all.map(_.key).toSet + val modelKeys = UploadLimits.Model.all.map(_.key).toSet + modelKeys should have size 4 + datasetKeys intersect modelKeys shouldBe empty + } + + "singleFileUploadMaxBytes" should "convert the declared MiB default to bytes when no row exists" in { + ifUnset("DATASET_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB")( + UploadLimits.Dataset.singleFileUploadMaxBytes shouldBe 20L * 1024L * 1024L + ) + // The regression: models used to resolve the dataset key and land on 20 MiB. + ifUnset("MODEL_SINGLE_FILE_UPLOAD_MAX_SIZE_MIB") { + UploadLimits.Model.singleFileUploadMaxBytes shouldBe 2048L * 1024L * 1024L + UploadLimits.Model.singleFileUploadMaxBytes should be > + UploadLimits.Dataset.singleFileUploadMaxBytes + } + } + + "ResourceStorage" should "hand each resource type its own limits" in { + ResourceStorage.Dataset.uploadLimits shouldBe UploadLimits.Dataset + ResourceStorage.Model.uploadLimits shouldBe UploadLimits.Model + } +} diff --git a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts index 4c764b99fd..13d10c4588 100644 --- a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts @@ -66,10 +66,10 @@ describe("AdminSettingsComponent", () => { favicon: "fav.ico", hub_enabled: "true", home_enabled: "false", - max_number_of_concurrent_uploading_file: "5", - single_file_upload_max_size_mib: "128", - max_number_of_concurrent_uploading_file_chunks: "7", - multipart_upload_chunk_size_mib: "64", + dataset_max_number_of_concurrent_uploading_file: "5", + dataset_single_file_upload_max_size_mib: "128", + dataset_max_number_of_concurrent_uploading_file_chunks: "7", + dataset_multipart_upload_chunk_size_mib: "64", csv_parser_max_columns: "4096", }); @@ -87,7 +87,7 @@ describe("AdminSettingsComponent", () => { it("keeps the initializer defaults for missing or unparsable values", () => { httpTestingController.expectOne(SETTINGS_URL).flush({ - single_file_upload_max_size_mib: "not-a-number", + dataset_single_file_upload_max_size_mib: "not-a-number", }); expect(component.logoData).toBeNull(); @@ -107,7 +107,7 @@ describe("AdminSettingsComponent", () => { it("preserves a legitimately stored 0 instead of falling back to the default", () => { httpTestingController.expectOne(SETTINGS_URL).flush({ - max_number_of_concurrent_uploading_file: "0", + dataset_max_number_of_concurrent_uploading_file: "0", csv_parser_max_columns: "0", }); @@ -299,10 +299,10 @@ describe("AdminSettingsComponent", () => { expect(req.request.body).toEqual({ value }); req.flush(null); }; - expectPut("max_number_of_concurrent_uploading_file", "3"); - expectPut("single_file_upload_max_size_mib", "20"); - expectPut("max_number_of_concurrent_uploading_file_chunks", "10"); - expectPut("multipart_upload_chunk_size_mib", "50"); + expectPut("dataset_max_number_of_concurrent_uploading_file", "3"); + expectPut("dataset_single_file_upload_max_size_mib", "20"); + expectPut("dataset_max_number_of_concurrent_uploading_file_chunks", "10"); + expectPut("dataset_multipart_upload_chunk_size_mib", "50"); expect(msgSuccess).toHaveBeenCalledWith("Dataset upload settings saved successfully."); }); @@ -346,10 +346,10 @@ describe("AdminSettingsComponent", () => { // Fail the last of the four PUTs so forkJoin errors with every request flushed. const keys = [ - "max_number_of_concurrent_uploading_file", - "single_file_upload_max_size_mib", - "max_number_of_concurrent_uploading_file_chunks", - "multipart_upload_chunk_size_mib", + "dataset_max_number_of_concurrent_uploading_file", + "dataset_single_file_upload_max_size_mib", + "dataset_max_number_of_concurrent_uploading_file_chunks", + "dataset_multipart_upload_chunk_size_mib", ]; keys.forEach((key, i) => { const req = httpTestingController.expectOne(updateUrl(key)); @@ -365,10 +365,10 @@ describe("AdminSettingsComponent", () => { component.resetDatasetSettings(); [ - "max_number_of_concurrent_uploading_file", - "single_file_upload_max_size_mib", - "max_number_of_concurrent_uploading_file_chunks", - "multipart_upload_chunk_size_mib", + "dataset_max_number_of_concurrent_uploading_file", + "dataset_single_file_upload_max_size_mib", + "dataset_max_number_of_concurrent_uploading_file_chunks", + "dataset_multipart_upload_chunk_size_mib", ].forEach(key => httpTestingController.expectOne(resetUrl(key)).flush(null)); expect(msgInfo).toHaveBeenCalledWith("Resetting dataset settings..."); }); diff --git a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts index 3714ac0de8..44b8fad73d 100644 --- a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts +++ b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts @@ -123,15 +123,18 @@ export class AdminSettingsComponent implements OnInit { tab => (this.sidebarTabs[tab] = settings[tab] === "true") ); this.maxConcurrentFiles = parseIntOrDefault( - settings["max_number_of_concurrent_uploading_file"], + settings["dataset_max_number_of_concurrent_uploading_file"], this.maxConcurrentFiles ); - this.maxFileSizeMiB = parseIntOrDefault(settings["single_file_upload_max_size_mib"], this.maxFileSizeMiB); + this.maxFileSizeMiB = parseIntOrDefault( + settings["dataset_single_file_upload_max_size_mib"], + this.maxFileSizeMiB + ); this.maxConcurrentChunks = parseIntOrDefault( - settings["max_number_of_concurrent_uploading_file_chunks"], + settings["dataset_max_number_of_concurrent_uploading_file_chunks"], this.maxConcurrentChunks ); - this.chunkSizeMiB = parseIntOrDefault(settings["multipart_upload_chunk_size_mib"], this.chunkSizeMiB); + this.chunkSizeMiB = parseIntOrDefault(settings["dataset_multipart_upload_chunk_size_mib"], this.chunkSizeMiB); this.csvMaxColumns = parseIntOrDefault(settings["csv_parser_max_columns"], this.csvMaxColumns); this.settingsLoaded = true; }, @@ -259,15 +262,18 @@ export class AdminSettingsComponent implements OnInit { const saveRequests = [ this.adminSettingsService.updateSetting( - "max_number_of_concurrent_uploading_file", + "dataset_max_number_of_concurrent_uploading_file", this.maxConcurrentFiles.toString() ), - this.adminSettingsService.updateSetting("single_file_upload_max_size_mib", this.maxFileSizeMiB.toString()), this.adminSettingsService.updateSetting( - "max_number_of_concurrent_uploading_file_chunks", + "dataset_single_file_upload_max_size_mib", + this.maxFileSizeMiB.toString() + ), + this.adminSettingsService.updateSetting( + "dataset_max_number_of_concurrent_uploading_file_chunks", this.maxConcurrentChunks.toString() ), - this.adminSettingsService.updateSetting("multipart_upload_chunk_size_mib", this.chunkSizeMiB.toString()), + this.adminSettingsService.updateSetting("dataset_multipart_upload_chunk_size_mib", this.chunkSizeMiB.toString()), ]; forkJoin(saveRequests) @@ -280,10 +286,10 @@ export class AdminSettingsComponent implements OnInit { resetDatasetSettings(): void { [ - "max_number_of_concurrent_uploading_file", - "single_file_upload_max_size_mib", - "max_number_of_concurrent_uploading_file_chunks", - "multipart_upload_chunk_size_mib", + "dataset_max_number_of_concurrent_uploading_file", + "dataset_single_file_upload_max_size_mib", + "dataset_max_number_of_concurrent_uploading_file_chunks", + "dataset_multipart_upload_chunk_size_mib", ].forEach(setting => this.adminSettingsService.resetSetting(setting).pipe(untilDestroyed(this)).subscribe({})); this.message.info("Resetting dataset settings..."); diff --git a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts index 5673e55878..10aed3dfd8 100644 --- a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts +++ b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts @@ -85,7 +85,7 @@ export class FilesUploaderComponent { ) { // A missing key or failed fetch keeps the initializer default above. this.adminSettingsService - .getPublicSetting("single_file_upload_max_size_mib") + .getPublicSetting("dataset_single_file_upload_max_size_mib") .pipe(untilDestroyed(this)) .subscribe({ next: value => (this.singleFileUploadMaxSizeMiB = parseIntOrDefault(value, this.singleFileUploadMaxSizeMiB)), diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts index 20570f4e95..7aae86e103 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts @@ -1227,9 +1227,9 @@ describe("DatasetDetailComponent behavior", () => { // A distinct value per key, so a setting that lands in the wrong field is // visible: 7 chunks and 2 files cannot stand in for one another. adminSettingsServiceStub.getPublicSetting.mockImplementation((key: string) => - key === "multipart_upload_chunk_size_mib" + key === "dataset_multipart_upload_chunk_size_mib" ? throwError(() => new Error("boom")) - : of(key === "max_number_of_concurrent_uploading_file_chunks" ? "7" : "2") + : of(key === "dataset_max_number_of_concurrent_uploading_file_chunks" ? "7" : "2") ); createComponent({ did: 5 }); @@ -1246,7 +1246,7 @@ describe("DatasetDetailComponent behavior", () => { it("leaves both concurrency limits untouched when their settings fail to load", () => { adminSettingsServiceStub.getPublicSetting.mockImplementation((key: string) => - key === "multipart_upload_chunk_size_mib" ? of("128") : throwError(() => new Error("boom")) + key === "dataset_multipart_upload_chunk_size_mib" ? of("128") : throwError(() => new Error("boom")) ); createComponent({ did: 5 }); diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts index 437f01d8bd..2f2e8b209e 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts @@ -585,21 +585,21 @@ export class DatasetDetailComponent implements OnInit { // silently stall the upload queue (`activeUploads < NaN` is always false). private loadUploadSettings(): void { this.adminSettingsService - .getPublicSetting("multipart_upload_chunk_size_mib") + .getPublicSetting("dataset_multipart_upload_chunk_size_mib") .pipe(untilDestroyed(this)) .subscribe({ next: value => (this.chunkSizeMiB = parseIntOrDefault(value, this.chunkSizeMiB)), error: () => {}, }); this.adminSettingsService - .getPublicSetting("max_number_of_concurrent_uploading_file_chunks") + .getPublicSetting("dataset_max_number_of_concurrent_uploading_file_chunks") .pipe(untilDestroyed(this)) .subscribe({ next: value => (this.maxConcurrentChunks = parseIntOrDefault(value, this.maxConcurrentChunks)), error: () => {}, }); this.adminSettingsService - .getPublicSetting("max_number_of_concurrent_uploading_file") + .getPublicSetting("dataset_max_number_of_concurrent_uploading_file") .pipe(untilDestroyed(this)) .subscribe({ next: value => (this.maxConcurrentFiles = parseIntOrDefault(value, this.maxConcurrentFiles)), diff --git a/sql/changelog.xml b/sql/changelog.xml index e86f869665..b57ec95ca4 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -119,6 +119,11 @@ <sqlFile path="sql/updates/41.sql"/> </changeSet> + <!-- Prefix the dataset upload-limit site settings with `dataset_` --> + <changeSet id="42" author="tanishqgandhi1908"> + <sqlFile path="sql/updates/42.sql"/> + </changeSet> + <!-- example changeSet <changeSet id="1" author="author"> <sqlFile path="sql/updates/1.sql"/> diff --git a/sql/updates/42.sql b/sql/updates/42.sql new file mode 100644 index 0000000000..eb82220041 --- /dev/null +++ b/sql/updates/42.sql @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +-- The dataset upload limits gained a `dataset_` prefix so that the model limits added +-- alongside them can be named symmetrically. site_settings rows are keyed by a +-- default.conf leaf's last path segment, so the renamed leaves are new row keys. +-- +-- Nothing orders this migration before the config-service seeder, so the prefixed row may +-- already exist at its default by the time this runs. The old row is then the only place +-- an admin's edit survives, and it is unreachable through the API (updateSetting and +-- resetSetting both reject a key with no default.conf entry). So carry the old value over +-- rather than skipping, then drop the old row. Renaming what is left is a no-op on a +-- second run, which keeps this idempotent. + +-- 1. The prefixed row already exists: the old row holds the value worth keeping. +UPDATE site_settings AS t +SET value = s.value, + updated_by = s.updated_by, + updated_at = s.updated_at +FROM site_settings AS s +WHERE s.key IN ( + 'single_file_upload_max_size_mib', + 'multipart_upload_chunk_size_mib', + 'max_number_of_concurrent_uploading_file', + 'max_number_of_concurrent_uploading_file_chunks' + ) + AND t.key = 'dataset_' || s.key; + +-- 2. Drop the old rows whose value has just been carried over. +DELETE FROM site_settings AS s +WHERE s.key IN ( + 'single_file_upload_max_size_mib', + 'multipart_upload_chunk_size_mib', + 'max_number_of_concurrent_uploading_file', + 'max_number_of_concurrent_uploading_file_chunks' + ) + AND EXISTS ( + SELECT 1 FROM site_settings AS t WHERE t.key = 'dataset_' || s.key + ); + +-- 3. No prefixed row was seeded: a plain rename preserves the value and its audit stamp. +UPDATE site_settings AS s +SET key = 'dataset_' || s.key +WHERE s.key IN ( + 'single_file_upload_max_size_mib', + 'multipart_upload_chunk_size_mib', + 'max_number_of_concurrent_uploading_file', + 'max_number_of_concurrent_uploading_file_chunks' + ); + +COMMIT;
