Yicong-Huang commented on code in PR #6116:
URL: https://github.com/apache/texera/pull/6116#discussion_r3607915570
##########
config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala:
##########
@@ -99,4 +117,114 @@ class ConfigResource {
// flags from the user-system.conf
"inviteOnly" -> UserSystemConfig.inviteOnly
)
+
+ // 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.
+ // 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"))
+
+ // Read side for the public keys in one payload, so the dashboard doesn't
+ // fire a request per key. Anonymous by design: these values render on the
+ // logged-out shell (custom logo/favicon, Hub/About sidebar entries), so
+ // gating them behind a login would blank the public landing pages.
+ @GET
+ @PermitAll
+ @Path("/settings/public")
+ def getPublicSettings: Map[String, String] = {
+ ctx
+ .select(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE)
+ .from(SITE_SETTINGS)
+ .where(SITE_SETTINGS.KEY.in(publicSettingKeys.asJava))
+ .fetchMap(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE)
+ .asScala
+ .toMap
+ }
+
+ // Management read over the site_settings table this service seeds at
+ // startup: every row, including the ones not exposed through
+ // /settings/public, in one payload for the admin settings page.
+ @GET
+ @RolesAllowed(Array("ADMIN"))
+ @Path("/settings")
+ def getAllSettings: Map[String, String] = {
+ ctx
+ .select(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE)
+ .from(SITE_SETTINGS)
+ .fetchMap(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE)
+ .asScala
+ .toMap
+ }
+
+ // Single-key management read, kept for API completeness alongside the bulk
+ // read above.
+ @GET
+ @RolesAllowed(Array("ADMIN"))
+ @Path("/settings/{key}")
+ def getSetting(@PathParam("key") keyParam: String): ConfigSettingPojo = {
+ ctx
+ .select(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE)
+ .from(SITE_SETTINGS)
+ .where(SITE_SETTINGS.KEY.eq(keyParam))
+ .fetchOneInto(classOf[ConfigSettingPojo])
+ }
+
+ @PUT
+ @RolesAllowed(Array("ADMIN"))
+ @Path("/settings/{key}")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def updateSetting(
+ @Auth currentUser: SessionUser,
+ @PathParam("key") keyParam: String,
+ setting: ConfigSettingPojo
+ ): Response = {
+ if (setting.settingValue == null) {
Review Comment:
Good call — applied in 6a5eb05. `updateSetting` now guards `if (setting ==
null || setting.settingValue == null)`, so an empty or malformed body returns a
400 instead of the 500 NPE, and I added a direct-call test for the null-body
path. While here it also now rejects a key with no `default.conf` entry with a
400 (mirroring `resetSetting`), so writes stay within the known-default
namespace.
##########
common/test/src/main/java/org/apache/texera/common/test/tags/IntegrationTest.java:
##########
@@ -17,7 +17,7 @@
* under the License.
*/
-package org.apache.texera.amber.tags;
+package org.apache.texera.common.test.tags;
Review Comment:
Agreed — taken out of scope in 6a5eb05. Reverted the `common/test`
(`TestUtil`) module extraction and kept `@IntegrationTest` in
`amber/src/test/integration/org/apache/texera/amber/tags/`; the amber specs,
`AMBER_TEST_FILTER`, CI, and labeler are back on the amber-local tag. The
shared-tag move will land as its own PR later.
--
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]
