Allow limits on user settings, with per-group overrides - #9594
Conversation
…eferences-enhancements
github-actions
Bot
added
a:webserver
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9594 +/- ##
==========================================
+ Coverage 88.18% 91.01% +2.82%
==========================================
Files 1563 1134 -429
Lines 60661 46757 -13904
Branches 1583 587 -996
==========================================
- Hits 53496 42557 -10939
+ Misses 6743 4053 -2690
+ Partials 422 147 -275
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds configurable group-scoped validation constraints for frontend user preferences, including an inactivity-threshold cap.
Changes:
- Adds dynamic preference validation and group overrides.
- Persists constraints in PostgreSQL JSONB storage.
- Adds error handling, migrations, and model/service tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Summary | Final review |
|---|---|---|
services/web/server/tests/unit/with_dbs/03/test_user_preferences_service.py |
Tests group constraint behavior. | No final comments. |
services/web/server/src/simcore_service_webserver/user_preferences/errors.py |
Defines preference validation errors. | No final comments. |
services/web/server/src/simcore_service_webserver/user_preferences/_service.py |
Applies group constraints during updates. | Critical (1 vote): Missing groups_extra_properties rows can cause preference writes to fail with an unhandled 500. |
services/web/server/src/simcore_service_webserver/user_preferences/_models.py |
Defines inactivity threshold constraints. | No final comments. |
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py |
Maps invalid values to HTTP 422. | Nit (3 votes): Add endpoint-level coverage for the invalid-value mapping. |
packages/postgres-database/src/simcore_postgres_database/utils_groups_extra_properties.py |
Exposes constraint data in aggregated properties. | No final comments. |
packages/postgres-database/src/simcore_postgres_database/models/groups_extra_properties.py |
Adds the JSONB constraint column. | No final comments. |
packages/postgres-database/src/simcore_postgres_database/migration/versions/9f24c8e1a3b7_add_frontend_preferences_constraints.py |
Migrates the database schema. | No final comments. |
packages/models-library/tests/test_user_preferences.py |
Tests dynamic constraint validation. | No final comments. |
packages/models-library/src/models_library/user_preferences.py |
Implements cached Pydantic validators. | No final comments. |
Suppressed comments (4)
packages/postgres-database/src/simcore_postgres_database/utils_groups_extra_properties.py:38
- This field is aggregated by
_merge_extra_properties_booleans, which keeps the first value for every non-boolfield. For a user in multiple standard groups, an earlier row with the migration's default{}therefore masks a later group's configured constraints; because the query orders only by group type, which row wins is not deterministic. Merge this constraint map per preference (with an explicit conflict precedence) and cover the multiple-standard-group case.
frontend_preferences_constraints: dict[str, dict[str, Any]]
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:27
- This changes an existing user-facing message (the placeholder changed from
frontend_preference_nametofrontend_preference_identifier) but leaves_versionunset. The translation pipeline uses_versionto track modified catalog entries, so bump the message version here.
user_message("Provided {frontend_preference_identifier} not found"),
services/web/server/src/simcore_service_webserver/user_preferences/_service.py:142
validate_valuedeliberately raisesInvalidValueConstraintsErrorfor unsupported or type-incompatible group overrides, but this block catches onlyValidationError. Since the new JSONB configuration is not schema-validated and this exception is absent from the REST map, one malformed group constraint makes preference PATCH requests escape as unhandled 500 errors; validate the stored configuration before use or handle this configuration failure explicitly.
try:
preference_class.validate_value(
value,
group_extra_properties.frontend_preferences_constraints.get(frontend_preference_identifier),
)
preference = preference_class.model_validate({"value": value})
except ValidationError as e:
raise FrontendUserPreferenceValueIsInvalidError(
frontend_preference_identifier=frontend_preference_identifier, value=value
) from e
services/web/server/tests/unit/with_dbs/03/test_user_preferences_service.py:219
- This fixture updates every
groups_extra_propertiesrow for the product, so Everyone, standard, and primary groups all receive the same map. It therefore does not exercise the new aggregation behavior when only one group supplies constraints or when group maps conflict; add group-specific setup/assertions so a regression in_aggregatecannot pass these tests.
groups_extra_properties.update()
.where(groups_extra_properties.c.product_name == product_name)
.values(frontend_preferences_constraints={_INACTIVITY_IDENTIFIER: constraints} if constraints else {})
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
GitHK
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:27
- This user-facing catalog message was modified without a version marker. Add
_version=1so the message key is tracked consistently with the other REST error messages.
This issue also appears on line 31 of the same file.
user_message("Provided {frontend_preference_identifier} not found"),
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:31
- The new 422 mapping is not covered at the REST boundary.
test_user_preferences_rest.pytests the 404 mapping but does not PATCH an invalid preference value, so a regression in the decorator/map could return a 500 or the wrong error envelope while the service-level tests still pass. Add an endpoint test foruserInactivityThresholdabove the allowed limit and assert the 422 response and message.
FrontendUserPreferenceValueIsInvalidError: HttpErrorInfo(
status.HTTP_422_UNPROCESSABLE_ENTITY,
user_message("The value {value} is not allowed for {frontend_preference_identifier}"),
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:31
- This new user-facing message omits
_version=N. The web-server error maps consistently versionuser_messagetemplates (for example,projects/_controller/_rest_exceptions.py:61), and the repository guideline requires versioning so translated catalog entries can evolve safely. Add the version argument here.
user_message("The value {value} is not allowed for {frontend_preference_identifier}"),
services/web/server/src/simcore_service_webserver/user_preferences/_service.py:113
- These overrides are read from the already aggregated group row, but the aggregation code does not merge this new dict: it returns the primary row immediately and treats non-boolean fields as
value1when combining standard rows. Thus a constraint configured only on a standard/everyone group is lost for users with a primary row, and competing standard-group values depend on row order. The tests avoid this by updating every row; aggregate this map with explicit precedence and add a group-specific test.
constraints = preference.get_value_constraints(
group_extra_properties.frontend_preferences_constraints.get(preference.preference_identifier)
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:23
- All
user_message(...)calls in this exceptions map should include an explicit_version=Nfor i18n/versioning consistency (many other web-server controllers do, e.g.services/web/server/src/simcore_service_webserver/products/_controller/rest_exceptions.py:16).
This issue also appears in the following locations of the same file:
- line 25
- line 29
CouldNotCreateOrUpdateUserPreferenceError: HttpErrorInfo(
status.HTTP_400_BAD_REQUEST,
user_message(
"Could not create or modify preferences",
),
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:28
- This user-facing
user_message(...)should include_version=Nfor i18n extraction/versioning consistency (see e.g.services/web/server/src/simcore_service_webserver/groups/_common/exceptions_handlers.py:27).
FrontendUserPreferenceIsNotDefinedError: HttpErrorInfo(
status.HTTP_404_NOT_FOUND,
user_message("Provided {frontend_preference_identifier} not found"),
),
services/web/server/src/simcore_service_webserver/user_preferences/_controller/rest/_rest_exceptions.py:32
- This user-facing
user_message(...)should include_version=Nfor i18n extraction/versioning consistency (many other web-server controllers do, e.g.services/web/server/src/simcore_service_webserver/tasks/_controller/_rest_exceptions.py:26).
FrontendUserPreferenceValueIsInvalidError: HttpErrorInfo(
status.HTTP_422_UNPROCESSABLE_ENTITY,
user_message("The value {value} is not allowed for {frontend_preference_identifier}"),
),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/postgres-database/src/simcore_postgres_database/utils_groups_extra_properties.py:39
frontend_preferences_constraintsis typed asdict[str, dict[str, Any]], but the rest of this PR explicitly tolerates malformed/misspelled overrides coming from the DB (e.g. values that are not mappings). Keeping this field narrow encourages callers to assume the nested value is always a dict, which is not guaranteed and is intentionally handled later (viaInvalidValueConstraintsError). Consider loosening the type to reflect the DB reality ("untrusted JSON"), e.g.dict[str, Any].
modified: datetime.datetime
enable_efs: bool
mount_data: bool
frontend_preferences_constraints: dict[str, dict[str, Any]]
packages/models-library/src/models_library/user_preferences.py:116
_VALUE_VALIDATOR_CLASSESis a process-global cache keyed by (preference class, serialized constraints). Because constraint overrides come from the DB and can change over time, this cache can grow without bound in a long-running service (each distinct override set creates a new Pydantic model class and is retained forever). Consider bounding the cache (simple eviction is fine) to avoid unbounded memory growth.
cache_key = (cls, json_dumps(constraints, sort_keys=True, default=str))
if cache_key not in _VALUE_VALIDATOR_CLASSES:
value_annotation = cls.model_fields["value"].annotation
try:
_VALUE_VALIDATOR_CLASSES[cache_key] = create_model(
| class UserInactivityThresholdFrontendUserPreference(FrontendUserPreference): | ||
| preference_identifier: PreferenceIdentifier = "userInactivityThreshold" | ||
| value: int = 30 * _MINUTE # in seconds | ||
| value_constraints: ClassVar[dict[str, Any]] = {"ge": 1 * _MINUTE, "le": 3 * _HOUR} |
There was a problem hiding this comment.
so that is the default.
minor and probably too late: why we do not use timedelta for all these timings? would that not be a bit easier?
There was a problem hiding this comment.
It's a bit late since the db is full of these values, also not staring forward to migrate. If we attempt doing so it will require a different PR. Also it requires frontend coordination to properly adapt
| class PreferenceConstraints(OutputSchema): | ||
| """Limits applying to a preference value, used by the frontend to render its widget.""" | ||
|
|
||
| ge: int | float | None = None |
There was a problem hiding this comment.
how do you handle conflicting constraints? e.g.
x>5 and x<3
There was a problem hiding this comment.
The same way Pydantic does, by doing nothing. This would be a configuration issue and will break.
python
Python 3.13.9 (main, Nov 17 2025, 09:40:22) [GCC 13.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pydantic import BaseModel, Field
...
... class Model(BaseModel):
... x: int = Field(gt=5, lt=3)
...
... Model(x=4)
...
Traceback (most recent call last):
File "<python-input-0>", line 6, in <module>
Model(x=4)
~~~~~^^^^^
File "/home/silenthk/work/pr-osparc-user-preferences-enhancements/.venv/lib/python3.13/site-packages/pydantic/main.py", line 263, in __init__
validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self)
pydantic_core._pydantic_core.ValidationError: 1 validation error for Model
x
Input should be less than 3 [type=less_than, input_value=4, input_type=int]
For further information visit https://errors.pydantic.dev/2.13/v/less_than| """Limits applying to a preference value, used by the frontend to render its widget.""" | ||
|
|
||
| ge: int | float | None = None | ||
| gt: int | float | None = None |
There was a problem hiding this comment.
This reminded me so much pydantci's internal code.
SEE https://github.com/pydantic/pydantic/blob/main/pydantic/fields.py#L67-L71
IMO it is worth exploring the possibility of using annotated_types library (already shipped with pydantic) and take advantage of all the goodies it comes with ...
There was a problem hiding this comment.
Short answer, current approach is better.
Long answer. You could use annotated_types, which do bring a better way to express the "constraints", at least in python.
But you still don't have anything to help you with serialisation to and from the database. It will only introduce more overhead just to use annotated_types.
I think this version is more compact and simple to maintain, for our limited purposes here.
…eferences-enhancements
…eferences-enhancements
|
What do these changes do?
What this does
User settings (things like the inactivity timeout, theme, or "ask before deleting") can now have limits attached to them, so that people can't save a value that doesn't make sense.
Each setting can declare a sensible default limit. As a first example, the inactivity timeout is capped at 3 hours.
Why
Until now any value could be saved for a setting, even nonsensical ones. There was also no way to give a particular team or customer different boundaries from everyone else.
What changes for users
What changes for administrators
Reference: limits you can configure
Limits go in the new
frontend_preferences_constraintscolumn of thegroups_extra_propertiestable. Each entry maps a setting name to the limits that apply to it:{ "userInactivityThreshold": { "ge": 60, "le": 21600 }, "themeName": { "pattern": "^(dark|light)$" } }These are all the limits available:
ge{"ge": 60}— no less than 60gt{"gt": 60}— 61 or abovele{"le": 21600}— no more than 21600lt{"lt": 21600}— 21599 or belowmin_length{"min_length": 3}max_length{"max_length": 5}multiple_of{"multiple_of": 60}— whole minutes onlypattern{"pattern": "^(dark|light)$"}Several limits can be combined for the same setting, and only the ones you specify replace the product defaults — anything you leave out keeps the default.
Below, a stripped down response from the
GET /v0/meendpoint, showcasing the default constraints for theuserInactivityThreshold, now enforced by the backend.{ "data": { "id": 1, ... "userInactivityThreshold": { "defaultValue": 1800, "value": 1800, "constraints": { "ge": 60, "le": 10800 } }, ... } }Related issue/s
idle=0functionality ⚠️ #9464How to test
Dev-ops