-
-
Notifications
You must be signed in to change notification settings - Fork 172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4a3ec183
i️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Badge Preserve the non-option configuration guides
For users following existing repository links into configuration.md, this replacement removes the guides for OIDC setup, custom components, configuration/web-root directories, connection initialization and cleanup scripts, migrations, and custom routes. The linked official reference is only a generated option table, so it cannot replace those sections; for example, existing links from README.md and CHANGELOG.md now lead to a page that no longer contains the referenced operational guidance. Keep the non-option sections in this file or move them to stable linked destinations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex restore the missing docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
- Restored the missing OpenID Connect setup documentation, including application address configuration and identity-provider examples. configuration.mdL36-L88
- Restored operational guides for custom components and configuration/web-root directories. configuration.mdL98-L145
- Restored connection initialization and cleanup scripts, migrations, and custom URL route documentation. configuration.mdL147-L226
- Committed the changes in commit
d28a960(docs: restore configuration guides) and created the pull request "Restore operational configuration guides."
Testing
- ✅
python3 - <<'PY' ... PY— verified all requested restored section headings are present. - ✅
git diff --check— completed without whitespace errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 685925e81c
i️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Badge Document the runtime CSP nonce placeholder
For users following the new generated reference, this points them to {{@csp_nonce}}, but the configuration parser only substitutes the literal {NONCE} placeholder (NONCE_PLACEHOLDER in src/webserver/content_security_policy.rs). A custom content_security_policy copied from this schema would therefore have no recognized nonce position, and is_enabled() would prevent SQLPage from sending the CSP header at all.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Badge Keep the CSP setting non-nullable
This schema now validates "content_security_policy": null, but the generated Rust field is ContentSecurityPolicyTemplate, whose Deserialize implementation delegates to String::deserialize; when the key is present as null, startup fails instead of using the default. Users relying on editor/schema validation can end up with a configuration that passes validation but SQLPage cannot load.
Useful? React with 👍 / 👎.
lovasoa
commented
Jun 16, 2026
@codex : no x-rust-type or anything fancy inside the schema definition. Use the standard to the best of its abilities, plus smart naming conventions and definitions to get a clean json schema definitions. If there are things that you see no way to cleanly encode in json schema or support through naming conventions, just use normal rust code. For instance, you could build a smaller sister struct that holds the additional parameters. Think about a clean design. the goal was refactoring and avoiding duplication between the code and docs, and we are already at a PR with 599 additions & 274 deletions !
Summary
- Removed Rust-specific schema extensions from the public configuration schema; the schema now uses standard JSON Schema properties without
x-rust-*metadata. sqlpage/sqlpage.schema.jsonL1-L18 - Moved Rust-specific AppConfig generation logic into
build.rs, using normal Rust code for field renames, serde attributes, and type overrides while inferring straightforward schema types from JSON Schema. build.rsL56-L79 build.rsL82-L179 - Fixed the CSP schema entry to document SQLPage’s actual
{NONCE}runtime placeholder and madecontent_security_policynon-nullable to match runtime deserialization. sqlpage/sqlpage.schema.jsonL229-L232 - Regenerated the official configuration reference so the CSP placeholder documentation matches the cleaned schema. examples/official-site/configuration.sqlL44-L48
- Committed the changes as
247d322(refactor: remove rust metadata from config schema) and created the PR "Remove Rust-specific metadata from configuration schema."
Testing
- ✅
cargo fmt --all -- --check - ✅
scripts/generate_configuration_reference.py --check - ✅
scripts/check_configuration_examples.py - ✅
git diff --check - ✅
python3 - <<'PY' ... PY— verified the schema has nox-rust-*metadata, uses{NONCE}, and keepscontent_security_policyas a string. ⚠️ cargo clippy --all-targets --all-features -- -D warnings— blocked by environment/toolchain issue:libsqlite3-sysbuild script uses unstablecfg_selectwithrustc 1.92.0.⚠️ cargo test— blocked by the samelibsqlite3-sys/ unstablecfg_selectbuild-script error.
Motivation
AppConfigwith a single machine-readable source so editor validation, runtime parsing, and documentation stay consistent.Description
sqlpage/sqlpage.schema.jsonthat declares types, defaults, descriptions andx-rust-*metadata for code generation.AppConfigRust struct at build time from the JSON Schema viabuild.rsand include it withinclude!(concat!(env!("OUT_DIR"), "/app_config.rs"));instead of the handwritten struct insrc/app_config.rs.scripts/generate_configuration_reference.pyto produceexamples/official-site/configuration.sqlfrom the schema andscripts/check_configuration_examples.pyto validate that every example JSON references the live schema, and convert existing example YAML files to schema-linked JSON with"$schema"set./configuration.sql) and add a unit testexample_json_configuration_deserializesto ensure the checked-in examplesqlpage/sqlpage.jsondeserializes intoAppConfig.Testing
cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warnings, which succeeded.cargo test, which completed with all unit and integration tests passing.scripts/generate_configuration_reference.py --checkandscripts/check_configuration_examples.py, and usednpx ajv-cli validate --strict=false --spec=draft2020 -s sqlpage/sqlpage.schema.json -d 'sqlpage/sqlpage.json' -d 'examples/**/sqlpage.json', all of which succeeded (AJV emitted format warnings only).cargo check --all-featuresto verify build-time code generation integration, which succeeded.Codex Task