Implement "all" endpoints supporting to configure whole instances#222
Draft
pathob wants to merge 18 commits into
Draft
Implement "all" endpoints supporting to configure whole instances#222pathob wants to merge 18 commits into
pathob wants to merge 18 commits into
Conversation
pathob
force-pushed
the
prepare-1.0
branch
2 times, most recently
from
409ad69 to
d34c610
Compare
pathob
force-pushed
the
all-endpoints
branch
3 times, most recently
from
6ce4812 to
95d6b8d
Compare
pathob
force-pushed
the
all-endpoints
branch
2 times, most recently
from
e10fca3 to
972184b
Compare
pathob
force-pushed
the
all-endpoints
branch
2 times, most recently
from
b29c8d8 to
98c8bbf
Compare
|
PUT / accepts a product-specific _AllModel and applies each present sub-field through the existing services. The outcome of every sub-field is reported in the response's status map; the overall HTTP status aggregates them (200 all success, 207 partial success, uniform client error code, or 500 on any server error). License keys in responses are redacted. Commons provides the shared framework: _AllResource/_AllService abstractions, ServiceResult, _AllModelStatus, ServiceResultUtil (including group-aware sub-entity status keys such as settings/custom-html), and LicenseKeyRedactor. Each product contributes its _AllModel, _AllServiceImpl, _AllResourceImpl (with product-typed OpenAPI response schemas), composite settings services, and Spring wiring. Composite settings services are not exposed as beans where their type would make by-type injection of the narrow settings interfaces ambiguous in the REST layer. Covered by unit tests for the framework, the composite default methods and the product services, plus functional tests for the _all endpoint (single- and multi-entity payloads, empty model, authentication and authorization) and a Confluence settings-branding functional test guarding the REST DI wiring.
pathob
commented
Jul 5, 2026
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @XmlRootElement | ||
| @XmlRootElement(name = "authentication-idps") |
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @XmlRootElement(name = SETTINGS + "-" + SETTINGS_BRANDING + "-" + COLOR_SCHEME) | ||
| @XmlRootElement(name = SETTINGS + "-" + SETTINGS_BRANDING) |
Contributor
Author
There was a problem hiding this comment.
Are we sure to change this?
Comment on lines
+1
to
+85
| package com.deftdevs.bootstrapi.commons.rest; | ||
|
|
||
| import com.deftdevs.bootstrapi.commons.model.type._AllModelAccessor; | ||
| import com.deftdevs.bootstrapi.commons.model.type._AllModelStatus; | ||
| import com.deftdevs.bootstrapi.commons.rest.api._AllResource; | ||
| import com.deftdevs.bootstrapi.commons.service.api._AllService; | ||
|
|
||
| import javax.ws.rs.core.Response; | ||
| import java.util.Map; | ||
|
|
||
| public abstract class _AbstractAllResourceImpl<_AllModel extends _AllModelAccessor> | ||
| implements _AllResource<_AllModel> { | ||
|
|
||
| static final int MULTI_STATUS = 207; | ||
|
|
||
| private final _AllService<_AllModel> allService; | ||
|
|
||
| public _AbstractAllResourceImpl( | ||
| final _AllService<_AllModel> allService) { | ||
|
|
||
| this.allService = allService; | ||
| } | ||
|
|
||
| @Override | ||
| public Response setAll( | ||
| final _AllModel allModel) { | ||
|
|
||
| final _AllModel result = allService.setAll(allModel); | ||
| final int overallStatus = computeOverallStatus(result.getStatus()); | ||
| return Response.status(overallStatus).entity(result).build(); | ||
| } | ||
|
|
||
| /** | ||
| * Aggregates per-sub-field statuses into a single HTTP response code: | ||
| * <ul> | ||
| * <li>All successful (or empty) → 200 OK.</li> | ||
| * <li>All entries share the same status code → that code.</li> | ||
| * <li>Any 5xx → 500 Internal Server Error.</li> | ||
| * <li>Mixed 2xx/4xx (or multiple distinct 4xx) → 207 Multi-Status.</li> | ||
| * </ul> | ||
| * 207 is used to signal "partial success" — callers must inspect the | ||
| * per-field {@code status} map in the response body to see which | ||
| * fields succeeded and which failed. | ||
| */ | ||
| static int computeOverallStatus( | ||
| final Map<String, _AllModelStatus> statusMap) { | ||
|
|
||
| if (statusMap == null || statusMap.isEmpty()) { | ||
| return Response.Status.OK.getStatusCode(); | ||
| } | ||
|
|
||
| boolean anyServerError = false; | ||
| boolean anyClientError = false; | ||
| boolean anySuccess = false; | ||
| Integer firstClientErrorCode = null; | ||
| boolean clientErrorCodesDiffer = false; | ||
|
|
||
| for (_AllModelStatus entry : statusMap.values()) { | ||
| final int code = entry.getStatus(); | ||
| if (code >= 500) { | ||
| anyServerError = true; | ||
| } else if (code >= 400) { | ||
| anyClientError = true; | ||
| if (firstClientErrorCode == null) { | ||
| firstClientErrorCode = code; | ||
| } else if (firstClientErrorCode != code) { | ||
| clientErrorCodesDiffer = true; | ||
| } | ||
| } else if (code >= 200 && code < 300) { | ||
| anySuccess = true; | ||
| } | ||
| } | ||
|
|
||
| if (anyServerError) { | ||
| return Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(); | ||
| } | ||
| if (anyClientError) { | ||
| if (anySuccess || clientErrorCodesDiffer) { | ||
| return MULTI_STATUS; | ||
| } | ||
| return firstClientErrorCode; | ||
| } | ||
| return Response.Status.OK.getStatusCode(); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Let's double check whether this implementation makes sense and is RFC conform
|
|
||
| public interface AuthenticationService<IB extends AbstractAuthenticationIdpModel, SB extends AuthenticationSsoModel> { | ||
|
|
||
| @SuppressWarnings("unchecked") |
Contributor
Author
There was a problem hiding this comment.
Is @SuppressWarnings("unchecked") our best option here (and below also)?
Comment on lines
+62
to
+63
| assertEquals(Response.Status.OK.getStatusCode(), result.getStatus().get("authentication/idps").getStatus()); | ||
| assertEquals(Response.Status.OK.getStatusCode(), result.getStatus().get("authentication/sso").getStatus()); |
Contributor
Author
There was a problem hiding this comment.
Prevent using these hard-coded strings
| final PermissionsGlobalModel permissionsGlobal = new PermissionsGlobalModel(); | ||
|
|
||
| doReturn(new ServiceResult<>(settings, | ||
| Collections.singletonMap("settings/general", _AllModelStatus.success()))) |
Comment on lines
+113
to
+114
| final Map<String, LicenseModel> redactedLicenses = | ||
| Collections.singletonMap("lice...nse1#abcd", LicenseModel.EXAMPLE_1); |
Contributor
Author
There was a problem hiding this comment.
Do we really want to hard-code this?
Comment on lines
+1
to
+31
| package it.com.deftdevs.bootstrapi.crowd.rest; | ||
|
|
||
| import com.deftdevs.bootstrapi.commons.model.MailServerSmtpModel; | ||
| import com.deftdevs.bootstrapi.commons.model.SettingsGeneralModel; | ||
|
|
||
| /** | ||
| * Example entities for functional tests. Unlike the {@code EXAMPLE_*} constants on the | ||
| * models (which document the API), these values describe the integration test | ||
| * environment — the mail server points at the greenmail service started for | ||
| * integration test runs (see {@code ci.yaml}), and the general settings are limited | ||
| * to the fields supported by Crowd. | ||
| */ | ||
| public final class FuncTestExamples { | ||
|
|
||
| public static final SettingsGeneralModel SETTINGS_GENERAL = SettingsGeneralModel.builder() | ||
| .baseUrl(SettingsGeneralModel.EXAMPLE_1.getBaseUrl()) | ||
| .title(SettingsGeneralModel.EXAMPLE_1.getTitle()) | ||
| .build(); | ||
|
|
||
| public static final MailServerSmtpModel MAIL_SERVER_SMTP_GREENMAIL = MailServerSmtpModel.builder() | ||
| .from("test@example.com") | ||
| .prefix("[Test]") | ||
| .host("localhost") | ||
| .port(3025) | ||
| .useTls(false) | ||
| .timeout(5000L) | ||
| .build(); | ||
|
|
||
| private FuncTestExamples() { | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Can we please get rid of this file again?
Comment on lines
+29
to
+61
| default ServiceResult<SettingsModel> setSettings(final SettingsModel settingsModel) { | ||
| final SettingsModel result = new SettingsModel(); | ||
| final Map<String, _AllModelStatus> status = new LinkedHashMap<>(); | ||
|
|
||
| if (settingsModel.getGeneral() != null) { | ||
| final String key = ServiceResultUtil.subEntityKey(SettingsGeneralModel.class); | ||
| try { | ||
| result.setGeneral(setSettingsGeneral(settingsModel.getGeneral())); | ||
| status.put(key, _AllModelStatus.success()); | ||
| } catch (Exception e) { | ||
| status.put(key, ServiceResultUtil.toErrorStatus(key, e)); | ||
| } | ||
| } | ||
| if (settingsModel.getSecurity() != null) { | ||
| final String key = ServiceResultUtil.subEntityKey(SettingsSecurityModel.class); | ||
| try { | ||
| result.setSecurity(setSettingsSecurity(settingsModel.getSecurity())); | ||
| status.put(key, _AllModelStatus.success()); | ||
| } catch (Exception e) { | ||
| status.put(key, ServiceResultUtil.toErrorStatus(key, e)); | ||
| } | ||
| } | ||
| if (settingsModel.getBanner() != null) { | ||
| final String key = ServiceResultUtil.subEntityKey(SettingsBannerModel.class); | ||
| try { | ||
| result.setBanner(setSettingsBanner(settingsModel.getBanner())); | ||
| status.put(key, _AllModelStatus.success()); | ||
| } catch (Exception e) { | ||
| status.put(key, ServiceResultUtil.toErrorStatus(key, e)); | ||
| } | ||
| } | ||
| return new ServiceResult<>(result, status); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
We have this code very similar for all settings services. Can we somehow dedup it?
Comment on lines
+43
to
+51
| public Map<String, LicenseModel> setLicenses( | ||
| final Map<String, LicenseModel> licenseInputs) { | ||
|
|
||
| final Map<String, LicenseModel> result = new LinkedHashMap<>(); | ||
| for (final String key : licenseInputs.keySet()) { | ||
| result.put(LicenseKeyRedactor.redact(key), addLicense(key)); | ||
| } | ||
| return result; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Can we dedup this function for all applications?
The implementation was identical in all three products: apply each license key and echo it back redacted. As a default method it lives next to its contract and the products only implement the product-specific addLicense.
SettingsGeneralModel.EXAMPLE_1_MINIMAL and MailServerSmtpModel.EXAMPLE_2_MINIMAL describe the field subset supported by all products, so the crowd functional tests no longer need their own fixture class or inline builders.
207 Multi-Status originates in RFC 4918 (WebDAV) where it implies an XML multistatus body; RFC 9110 permits any registered code, and 207 with a JSON body carrying per-part statuses is established bulk-endpoint practice. The alternatives are worse: plain 200 hides partial failure from status-code-only clients, 4xx/5xx misrepresents the successfully applied sub-fields.
Confluence and Crowd had a SettingsServiceImpl next to a composite *SettingsServiceImpl that only delegated to it, unlike Jira where one implementation implements the whole product settings interface. Each product now has a single settings implementation and a single settings bean; the branding implementation stays internal to it, keeping by-type injection of the narrow settings interfaces unambiguous in the REST layer. Also drops the double blank lines in the Confluence implementation.
Every binding of the IdP and SSO type parameters across the authentication interfaces, abstract implementations and resources used the same two types, so the generics carried no information and forced unchecked casts in the composite default methods. With plain wildcard signatures the casts and their @SuppressWarnings disappear; defensive LinkedHashMap copies bridge the wildcard maps into the wire model.
Status-map keys previously mirrored REST path segments and were derived from @XmlRootElement names by dash-splitting, which forced root-element renames, a group-aware special case for settings/custom-html and hardcoded key strings in tests. They now mirror the JSON structure of the request instead: composite services record their sub-fields under the field's own name (general, smtp, idps) via a shared ServiceResultUtil.setSubEntity helper that also removes the duplicated try/catch blocks, and the _all service prefixes them with the composite's field name when merging (settings/general, mailServer/smtp). Top-level fields are keyed by their field name (permissionsGlobal, applicationLinks). No layer needs to know its parents, the dash-splitting utilities and the questioned root-element changes are gone, and a status key always matches the part of the payload it refers to.
The previous aggregation was asymmetric: a 5xx part turned the whole response into a 500 even though other parts were applied, while 4xx parts in a mixed outcome yielded 207. Following RFC 4918's own convention, where a multistatus is used whenever parts end differently regardless of status class, the rule is now uniform: the unanimous code when all parts agree (all 200 → 200, all failed alike → that 4xx/5xx), 207 for any mix.
_AbstractAllModel carries the configuration fields every product supports (settings via type parameter, directories, applicationLinks, licenses, mailServer) plus the response status map; the Confluence and Jira _AllModel were byte-identical and now only add their product-specific fields, as does Crowd. AbstractSettingsModel is the canonical settings group schema (general, security, branding) extended by Confluence; Jira and Crowd keep their reduced field sets. The affected models move from @builder to @SuperBuilder, so the two remaining all-args constructions become builders.
Sub-models declare their parent with @SubEntityOf(Parent.class); FieldNames resolves the serialized field name reflectively from the parent's (possibly inherited and generic) field declarations, so a composite update records a sub-field's status by passing the sub-model class alone: setSubEntity(status, MailServerPopModel.class, ...). The _all base service derives its top-level field names the same way from the concrete _AllModel bound by the product subclass, and prefixes composite sub-statuses with the composite's field name. No string keys, field constants or annotations with name segments remain anywhere; tests derive expected keys with FieldNames.pathOf(_AllModel.class, leaf.class), and any model change that breaks or ambiguates a derivation fails fast in the unit tests.
setSettingsSecurity mutated the Settings copy returned by getGlobalSettings() but never committed it, so PUT /settings/security (and the security sub-field of the _all endpoint) reported success without persisting anything. The _all services also collected statuses in a HashMap, discarding the request-mirroring order the composites produce; LinkedHashMap keeps the response status map in request order.
The underscore prefix leads for visual separation of the _all family, matching _AbstractAllModel, _AbstractAllServiceImpl and _AbstractAllResourceImpl.
A missing or null request body now yields 400 instead of an NPE-driven 500. FieldNames.pathOf collects all candidate paths before deciding, so nested ambiguity is reported instead of silently treated as absence, a visited set guards against model cycles, and enum-typed fields are not descended into. getAuthenticationIdps uses the same curated duplicate-name error as findIdpConfigByName instead of an uncontrolled Collectors.toMap exception.
The _AllResource interface no longer carries an @operation block: OpenAPI only reads the overriding product methods, so the interface copy was shadowed and could only drift. The response descriptions now state that a unanimous sub-field failure returns that status code with the same body, the 207 wording covers mixed failures without successes, and Crowd's operation notes that POP cannot be applied. Removes the unused mail-templates/session-config/trusted-proxies path constants, reverts the vestigial login-page root-element rename, and makes setEntity/setEntities void since no caller used their return value. Regenerates the docs, whose _AllModel field order had drifted after the base-model extraction.
ServiceResultUtil.setSubEntity and the _AbstractAllServiceImpl helpers duplicated the same null-skip, apply, success-or-error-status body and differed only in how the status key is derived. All of them now delegate to a single key-taking ServiceResultUtil.setEntity core.
207 Multi-Status belongs to the 2xx success class, so scripted callers would treat partial failure as success unless they inspect the body. The overall response code is now 200 only when every sub-field succeeded, otherwise the highest (most severe) sub-field status code. The apply is declarative and not transactional: succeeded sub-fields stay applied, re-submitting the same payload is safe, and the per-sub-field status map still distinguishes partial from total failure.
Looking the field up by type coupled the key to 'the only List<String> field of the model' and would break as soon as another such field is added. A getter reference (_AllModel::getTrustedProxies) assigned to a SerializableFunction carries its method metadata in the compiler's SerializedLambda, from which FieldNames now derives the backing field's serialized name - still fully programmatic, and refactoring-safe.
A single base could not fit all products: Jira has no branding, Crowd has neither security nor color-scheme branding and names its login-page branding field the same. The base is now layered by support - AbstractSettingsModel (general) is extended by AbstractSettingsSecurityModel (security), which is extended by AbstractSettingsBrandingModel (color-scheme branding). Confluence extends the branding level, Jira the security level and Crowd the base, so each @SubEntityOf anchor points at the class that declares the field and no product model aligns by convention or advertises sub-fields it cannot apply.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.