feat(config-service): move site_settings API from texera-web to config-service by Yicong-Huang · Pull Request #6116 · apache/texera

Yicong-Huang · 2026-07-04T21:58:47Z

What changes were proposed in this PR?

Moves the site_settings API from texera-web's AdminSettingsResource into config-service's ConfigResource — config-service already seeds the table and has the JWT stack registered. New surface:

GET /config/settings/public — anonymous, all user-visible keys (branding, sidebar tabs, upload limits) in one payload, replacing the request-per-key pattern. Anonymous by design: these values render on the logged-out shell, and the SPA login never recreates the dashboard shell, so gating them would blank public pages and leave freshly logged-in users unbranded until a refresh.
GET /config/settings — ADMIN, all rows in one payload; the admin settings page now loads with one request instead of 20.
GET/PUT /config/settings/{key}, POST /config/settings/reset/{key} — ADMIN; a null value on PUT is now a 400 instead of a silent no-op.

The public whitelist is derived from the gui/dataset sections of default.conf (new DefaultsConfig.keysUnderSections, pinned by a spec) instead of a hand-maintained list; keys outside those sections stay management-only. These endpoints complement the HOCON-backed /config/* reads (deploy-time, frozen per JVM) with no key overlap.

All site_settings access — resource queries, startup seeder, dao.SiteSettings — now uses the generated jOOQ SITE_SETTINGS table instead of string-based lookups, and ConfigService declares its direct DAO dependency.

Also introduces common/test (sbt project TestUtil), a TEST-ONLY util module carrying the shared @org.apache.texera.common.test.tags.IntegrationTest ScalaTest tag (class-level Java annotation + per-test IntegrationTestTag) for specs that need real infrastructure such as Postgres. ConfigResourceSpec is tagged; amber's six integration specs, its AMBER_TEST_FILTER wiring, and the CI/labeler references migrate off amber's private copy of the tag, which is deleted.

Frontend: AdminSettingsService moves to /api/config/settings (existing routing covers it; no proxy/nginx/k8s changes). getPublicSetting reads from one shared cached request, is typed string | null, and consumers fall back to their defaults on a missing key or failed fetch. The service invalidates its cache on save/reset instead of relying on the admin page's post-save reload; the per-key frontend getSetting is gone.

Any related issues, documentation, discussions?

Closes #6115

How was this PR tested?

sbt ConfigService/test — one ConfigResourceSpec (tagged @IntegrationTest) covers the HTTP auth gates and the endpoint bodies against embedded Postgres, including the whitelist-derivation pin and null-value 400; DefaultsConfigSpec covers keysUnderSections. Amber test sources compile against the migrated tag.
Frontend: new admin-settings.service.spec.ts (cache sharing, error retry, invalidation on save/reset); component specs cover the admin page's bulk load (defaults on missing/unparsable values, error toast) and the dashboard/uploader null-and-error fallbacks; tsc --noEmit and prettier clean.
Manual end-to-end on the local dev stack at the current head: anonymous GET /settings/public serves the full whitelisted map through the dev proxy, GET /settings and GET /settings/{key} reject anonymous callers with 401, old /api/admin/settings/{key} 404s.

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

Generated-by: Claude Code (claude-fable-5)

…b to config-service Fold AdminSettingsResource's GET/PUT/reset endpoints into ConfigResource as /config/settings/{key}, so the service that seeds site_settings also owns its read/write API. PUT and reset stay ADMIN-only; the single-key GET keeps REGULAR+ADMIN because non-admin pages (dashboard logo/tabs, dataset upload limits) read it. The frontend service now rides the existing /api/config routing, so no proxy or nginx changes are needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-07-04T21:59:04Z

Automated Reviewer Suggestions

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

Contributors with relevant context: @Ma77Ball, @xuang7, @aglinxinyuan
You can notify them by mentioning @Ma77Ball, @xuang7, @aglinxinyuan in a comment.

codecov-commenter · 2026-07-04T22:01:35Z

Codecov Report

❌ Patch coverage is 74.35897% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.54%. Comparing base (1366b87) to head (6a5eb05).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ain/scala/org/apache/texera/dao/SiteSettings.scala	0.00%	17 Missing ⚠️
...mponent/admin/settings/admin-settings.component.ts	65.38%	8 Missing and 1 partial ⚠️
...g/apache/texera/common/config/DefaultsConfig.scala	84.61%	0 Missing and 2 partials ⚠️
...pache/texera/service/resource/ConfigResource.scala	94.11%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6116      +/-   ##
============================================
+ Coverage     70.45%   70.54%   +0.09%     
- Complexity     3407     3435      +28     
============================================
  Files          1142     1145       +3     
  Lines         44876    44971      +95     
  Branches       4955     4975      +20     
============================================
+ Hits          31616    31727     +111     
+ Misses        11618    11590      -28     
- Partials       1642     1654      +12

Flag	Coverage Δ		*Carryforward flag
access-control-service	`70.00% <ø> (ø)`
agent-service	`76.76% <ø> (ø)`		Carriedforward from 22b97a0
amber	`66.77% <36.66%> (+0.08%)`	⬆️
computing-unit-managing-service	`17.72% <ø> (ø)`
config-service	`66.66% <94.11%> (+14.35%)`	⬆️
file-service	`66.80% <ø> (ø)`
frontend	`69.42% <83.01%> (+0.12%)`	⬆️
notebook-migration-service	`78.94% <ø> (ø)`
pyamber	`91.53% <ø> (ø)`		Carriedforward from 22b97a0
workflow-compiling-service	`55.14% <ø> (ø)`

*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.

…ead of string-based DSL Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-07-04T22:08:37Z

⚠️ Benchmark changes need a look

🟢 4 better · 🔴 3 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 1366b87 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	379	0.231	26,582/29,472/29,472 us	🔴 +18.2% / 🔴 +124.4%
🟢	bs=100 sw=10 sl=64	805	0.491	124,824/137,166/137,166 us	🟢 -12.1% / 🔴 +38.4%
⚪	bs=1000 sw=10 sl=64	929	0.567	1,076,011/1,118,699/1,118,699 us	⚪ within ±5% / 🔴 +18.2%

Baseline details

Latest main 1366b87 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	379 tuples/sec	437 tuples/sec	831.47 tuples/sec	-13.3%	-54.4%
bs=10 sw=10 sl=64	MB/s	0.231 MB/s	0.267 MB/s	0.507 MB/s	-13.5%	-54.5%
bs=10 sw=10 sl=64	p50	26,582 us	22,496 us	11,844 us	+18.2%	+124.4%
bs=10 sw=10 sl=64	p95	29,472 us	31,029 us	14,359 us	-5.0%	+105.2%
bs=10 sw=10 sl=64	p99	29,472 us	31,029 us	18,888 us	-5.0%	+56.0%
bs=100 sw=10 sl=64	throughput	805 tuples/sec	816 tuples/sec	1,089 tuples/sec	-1.3%	-26.1%
bs=100 sw=10 sl=64	MB/s	0.491 MB/s	0.498 MB/s	0.665 MB/s	-1.4%	-26.1%
bs=100 sw=10 sl=64	p50	124,824 us	119,735 us	92,503 us	+4.3%	+34.9%
bs=100 sw=10 sl=64	p95	137,166 us	156,014 us	99,121 us	-12.1%	+38.4%
bs=100 sw=10 sl=64	p99	137,166 us	156,014 us	112,422 us	-12.1%	+22.0%
bs=1000 sw=10 sl=64	throughput	929 tuples/sec	928 tuples/sec	1,117 tuples/sec	+0.1%	-16.8%
bs=1000 sw=10 sl=64	MB/s	0.567 MB/s	0.566 MB/s	0.682 MB/s	+0.2%	-16.8%
bs=1000 sw=10 sl=64	p50	1,076,011 us	1,080,432 us	910,046 us	-0.4%	+18.2%
bs=1000 sw=10 sl=64	p95	1,118,699 us	1,117,324 us	950,716 us	+0.1%	+17.7%
bs=1000 sw=10 sl=64	p99	1,118,699 us	1,117,324 us	985,832 us	+0.1%	+13.5%

Raw CSV

config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,527.42,200,128000,379,0.231,26581.67,29472.23,29472.23
1,100,10,64,20,2484.45,2000,1280000,805,0.491,124824.09,137166.15,137166.15
2,1000,10,64,20,21530.01,20000,12800000,929,0.567,1076010.60,1118699.32,1118699.32

…dded Postgres Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ConfigService main code uses SqlServer and the generated jOOQ tables directly but only got DAO transitively through Auth; declare it like the other services that touch the database do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…dmin single-key GET Non-admin pages only need 19 whitelisted keys (branding, sidebar tabs, upload limits); serve those in one payload via GET /config/settings/public (REGULAR+ADMIN) and make the arbitrary single-key GET ADMIN-only. The frontend reads public settings through one cached request instead of a request per key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

xuang7

Generally LGTM! I left one comment.

…etch Address review: if the first /config/settings/public request errors, shareReplay(1) would replay that error to every consumer with no retry. Drop the cached observable on error so the next read retries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ived whitelist, bulk admin read - /config/settings/public is @permitAll again: its values (custom logo, favicon, Hub/About sidebar entries) render on the logged-out shell, so gating them behind a login blanked public landing pages and, because the SPA login never recreates the dashboard shell, left even freshly logged-in users without branding until a manual refresh. - Derive the public whitelist from the gui/dataset sections of default.conf (new DefaultsConfig.keysUnderSections) instead of a hand-maintained parallel list; a CrudSpec test pins the derived set so visibility changes still surface in review. - Add ADMIN-only GET /config/settings (all rows in one payload); the admin settings page now loads with one request instead of 20 per-key GETs, and the frontend's per-key getSetting is gone. - PUT /config/settings/{key} rejects a null value with 400 instead of a silent 200 no-op. - Converge all site_settings access on the generated SITE_SETTINGS table: the ConfigService seeder and dao SiteSettings drop their string-based DSL.table/DSL.field lookups. - AdminSettingsService invalidates its public-settings cache in updateSetting/resetSetting rather than relying on the admin page's window.location.reload; getPublicSetting is typed string | null and consumers keep their defaults on a missing key or failed fetch instead of parseInt(null) NaN; new service spec covers the cache/retry paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

# Conflicts: # frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts # frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts

…Service API dataset-detail and files-uploader specs (from apache#6297-era main) mock the removed per-key getSetting; rename the stubs to getPublicSetting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…CrudSpec The spec tests ConfigResource (there is no ConfigSettings class); pair it with ConfigResourceAuthSpec — Auth pins the gates, Crud pins the bodies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…Spec Repo convention is <SourceClass>Spec; there is no ConfigResourceCrud class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…Spec There is no ConfigResourceAuth source class; one ConfigResourceSpec now covers both the HTTP auth gates (ResourceExtension + JwtAuthFilter) and the endpoint bodies (direct calls against the embedded database). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- DefaultsConfigSpec: keysUnderSections section filtering, union, and unknown-section cases. - admin-settings.component: bulk load populates every form field, keeps initializer defaults on missing/unparsable values, surfaces a load failure through the message service. - dashboard.component: loadLogos/loadTabs apply values, keep defaults on missing keys, and swallow fetch errors without throwing. - files-uploader / dataset-detail: missing public settings keep the hardcoded upload defaults instead of NaN. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

New TestUtil module (common/test) carries the ScalaTest tag annotation as main-scope sources, so any module can mark specs that need real external infrastructure via dependsOn(TestUtil % "test"). ConfigResourceSpec (embedded Postgres) is the first user. No CI filter is wired yet — tagged specs keep running in their module's test job; amber keeps its own org.apache.texera.amber.tags.IntegrationTest until migrated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…va TestUtil module Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

taggedAs IntegrationTestTag marks a single test inside an otherwise unit-level spec (same tag name as the class-level annotation, so -l/-n filters match either). With a Scala source present, doc runs scaladoc, so the javadoc javacOptions workaround is dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…eUnused runs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Delete amber's own org.apache.texera.amber.tags.IntegrationTest and point its six integration specs, the AMBER_TEST_FILTER wiring, and the CI/labeler references at the shared org.apache.texera.tags.IntegrationTest from the TestUtil module. Document common/test as a TEST-ONLY util module: main-scope sources consumed via dependsOn(TestUtil % "test"), never a Compile-scope dependency, never bundled into a dist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Package now mirrors the module path (common/test), keeping test-only utilities visibly namespaced apart from production org.apache.texera code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aglinxinyuan · 2026-07-18T00:24:04Z

 */

-package org.apache.texera.amber.tags;
+package org.apache.texera.common.test.tags;


This move should be done in a separate PR?

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.

Ma77Ball · 2026-07-18T06:53:55Z

+      @PathParam("key") keyParam: String,
+      setting: ConfigSettingPojo
+  ): Response = {
+    if (setting.settingValue == null) {


I recommend checking the request body (setting itself) for an empty value. This will better help guard from malformed request bodies and avoid sending the wrong error message (a 500 rather than a 400).

Suggested change

if (setting.settingValue == null) {

if (setting == null || setting.settingValue == null) {

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.

Ma77Ball

Overall LGTM! I tested and ran the PR on Docker and k8s, confirming everything is running, and added a small nit to do above.

@Ma77Ball

- updateSetting: guard a null/empty request body -> 400 instead of a 500 NPE, and reject keys with no default.conf entry (per @Ma77Ball). - Move site_settings writes into dao.SiteSettings (upsert / insertIfAbsent) so the resource and the startup seeder share one column/audit-stamp shape. - DefaultsConfig: fail fast on colliding leaf keys; add an operator section (csv_parser_max_columns) deliberately kept out of the anonymous /config/settings/public whitelist. - Frontend: parseIntOrDefault (preserve a stored 0), block admin saves until a load has succeeded, and exclude /config/settings/public from JWT injection so the anonymous read is never sent a token. - Keep the shared @IntegrationTEST tag in amber and drop the common/test module extraction from this PR, to land separately (per @aglinxinyuan). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

aglinxinyuan

LGTM!

github-actions Bot assigned Yicong-Huang Jul 4, 2026

github-actions Bot added engine frontend Changes related to the frontend GUI platform Non-amber Scala service paths labels Jul 4, 2026

Yicong-Huang requested review from kunwp1 and xuang7 July 4, 2026 22:00

refactor(config-service): use generated jOOQ SITE_SETTINGS table inst…

2b7ed36

…ead of string-based DSL Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

test(config-service): cover /config/settings positive paths with embe…

86b76b7

…dded Postgres Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions Bot added dependencies Pull requests that update a dependency file common labels Jul 4, 2026

Yicong-Huang and others added 2 commits July 4, 2026 15:25

xuang7 approved these changes Jul 5, 2026

View reviewed changes

Comment thread frontend/src/app/dashboard/service/admin/settings/admin-settings.service.ts Outdated

Yicong-Huang and others added 12 commits July 4, 2026 19:36

Merge branch 'main' into move-admin-settings-to-config-service

dbe6012

# Conflicts: # frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts # frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts

test(frontend): adapt specs merged from main to the new AdminSettings…

00e06d0

…Service API dataset-detail and files-uploader specs (from apache#6297-era main) mock the removed per-key getSetting; rename the stubs to getPublicSetting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

test(config-service): rename ConfigResourceCrudSpec to ConfigResource…

eef437b

…Spec Repo convention is <SourceClass>Spec; there is no ConfigResourceCrud class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix(build): javadoc rejects inherited javac lint flags in the pure-Ja…

cb4bcf5

…va TestUtil module Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix(build): give TestUtil the shared scalac options so scalafix Remov…

1243252

…eUnused runs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Yicong-Huang requested a review from aglinxinyuan July 18, 2026 00:11

github-actions Bot added ci changes related to CI amber-integration labels Jul 18, 2026

refactor(test): move shared tags to org.apache.texera.common.test.tags

22b97a0

Package now mirrors the module path (common/test), keeping test-only utilities visibly namespaced apart from production org.apache.texera code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aglinxinyuan requested changes Jul 18, 2026

View reviewed changes

Ma77Ball suggested changes Jul 18, 2026

View reviewed changes

Ma77Ball approved these changes Jul 18, 2026

View reviewed changes

github-actions Bot removed ci changes related to CI amber-integration labels Jul 18, 2026

Yicong-Huang changed the title ~~refactor(config-service): move site_settings write API from texera-web to config-service~~ feat(config-service): move site_settings API from texera-web to config-service Jul 18, 2026

Yicong-Huang requested a review from aglinxinyuan July 18, 2026 07:38

aglinxinyuan approved these changes Jul 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(config-service): move site_settings API from texera-web to config-service#6116

feat(config-service): move site_settings API from texera-web to config-service#6116
Yicong-Huang wants to merge 20 commits into
apache:mainfrom
Yicong-Huang:move-admin-settings-to-config-service

Yicong-Huang commented Jul 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

xuang7 left a comment

Uh oh!

Uh oh!

aglinxinyuan Jul 18, 2026

Uh oh!

Yicong-Huang Jul 18, 2026

Uh oh!

Ma77Ball Jul 18, 2026

Uh oh!

Yicong-Huang Jul 18, 2026

Uh oh!

Ma77Ball left a comment

Uh oh!

aglinxinyuan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if (setting.settingValue == null) {
	if (setting == null \|\| setting.settingValue == null) {

Uh oh!

Conversation

Yicong-Huang commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this PR?

Any related issues, documentation, discussions?

How was this PR tested?

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

Uh oh!

github-actions Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Reviewer Suggestions

Uh oh!

codecov-commenter commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Benchmark changes need a look

Uh oh!

xuang7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aglinxinyuan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Yicong-Huang Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Ma77Ball Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Yicong-Huang Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Ma77Ball left a comment

Choose a reason for hiding this comment

Uh oh!

aglinxinyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Yicong-Huang commented Jul 4, 2026 •

edited

Loading

github-actions Bot commented Jul 4, 2026 •

edited

Loading

codecov-commenter commented Jul 4, 2026 •

edited

Loading

github-actions Bot commented Jul 4, 2026 •

edited

Loading