Add v1 REST API deprecation control with build-time flag#5324
Open
fmarco76 wants to merge 1 commit intodogtagpki:masterfrom
Open
Add v1 REST API deprecation control with build-time flag#5324fmarco76 wants to merge 1 commit intodogtagpki:masterfrom
fmarco76 wants to merge 1 commit intodogtagpki:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds build-time and runtime controls for the v1 REST API lifecycle (enabled|deprecated|disabled|dropped). Build tooling and packaging gate compilation and dependencies; runtime helper and filters implement deprecation/disabled behavior; application initializers consult the helper to conditionally short‑circuit v1 registrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as SubsystemApplication
participant Helper as V1ApiStatusHelper
participant Registry as JAXRS_Registry
participant Disabled as V1ApiDisabledResource
Client->>App: startup or incoming v1 request
App->>Helper: checkV1ApiStatus(subsystem, classes, logger)
alt status == disabled
Helper->>Registry: add V1ApiDisabledResource.class
Registry->>Disabled: register catch‑all
App-->>Client: subsequent v1 requests → 410 Gone (disabled response)
else status == deprecated
Helper->>Registry: add V1ApiDeprecationFilter.class
Registry->>App: continue with registrations
App-->>Client: v1 endpoints active + deprecation headers
else status == enabled
Helper-->>App: no changes
App->>Registry: register v1 resources & interceptors
App-->>Client: v1 endpoints active (normal flow)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a build-time and runtime control mechanism for the v1 REST API, allowing it to be enabled, deprecated, disabled, or completely dropped from the build. Review feedback identifies significant code duplication in the status-checking logic across multiple JAX-RS Application classes, recommending a refactor into a shared utility. Additionally, the review points out duplicate imports in several files, missing exception logging in the new disabled resource handler, and a URL inconsistency between the implementation and the documentation.
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.java`:
- Around line 49-69: The runtime v1ApiStatus value (from
System.getProperty("v1.api.status", getDefaultV1ApiStatus())) must be validated
before falling through to the enabled path: accept only known values ("enabled",
"disabled", "deprecated"); if the property contains any other value (including
empty or typos) log a warning via logger.warn that the override is invalid and
then reset v1ApiStatus = getDefaultV1ApiStatus() (or explicitly set to
"enabled"/"disabled"/"deprecated" as your policy requires) so the code will not
silently enable v1; apply the same validation guard to the sibling v1
*Application classes and reference getDefaultV1ApiStatus(), the local
v1ApiStatus variable, and the classes.add(...V1ApiDisabledResource.class) branch
to locate where to add the check.
In `@base/pom.xml`:
- Around line 57-66: The broad exclude pattern **/*Resource.java is removing
non-v1 types; narrow it to only v1 sources by changing that exclude to target
the v1 REST tree (e.g. use **/rest/v1/*Resource.java or
**/rest/v1/**/*Resource.java) so it complements the existing **/rest/v1/**, and
keep the other excludes for PKIService.java, SubsystemService.java and
v1/PKIApplication.java unchanged.
In
`@base/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.java`:
- Around line 56-76: Validate the runtime v1ApiStatus value up front: treat
"dropped" the same as "disabled" by mapping v1ApiStatus == "dropped" ->
"disabled", and otherwise ensure v1ApiStatus is one of the allowed values
("enabled" (or default), "deprecated", "disabled"); if it's not, log an explicit
error including the invalid value and fail fast (throw an
IllegalArgumentException or similar) instead of proceeding to normal
registration. Apply this same validation/mapping logic where v1ApiStatus and
getDefaultV1ApiStatus() are used (e.g., PKIApplication and the other
*Application classes), and keep the existing behavior of registering
V1ApiDisabledResource.class when the value resolves to "disabled".
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.java`:
- Around line 3-9: Add an explicit `@OPTIONS` handler in the V1ApiDisabledResource
class so OPTIONS requests also return HTTP 410 Gone; import javax.ws.rs.OPTIONS
and implement a method (e.g., options() or handleOptions()) annotated with
`@OPTIONS` and `@Path`("/") (or no Path if class-level path covers it) that returns
Response.status(Response.Status.GONE).entity(...appropriate 410
payload...).build() so CORS preflight and explicit OPTIONS calls follow the same
410 contract as the other HTTP methods.
In `@docs/changes/v11.10.0/Packaging-Changes.adoc`:
- Around line 59-89: Add an explicit note describing the runtime override
mechanism: state that the build-time flag --v1-api-status is mirrored at runtime
via the JVM system property -Dv1.api.status and list the supported runtime
values ("deprecated", "disabled", "dropped") and their effects, and mention that
build.properties is the resource used to set the default if -Dv1.api.status is
not provided; update the "Migration Path" section (around the steps mentioning
rollback) to include this -Dv1.api.status example and clarify that changing it
at runtime does not require a rebuild.
In `@pki.spec`:
- Around line 283-291: The spec currently only gates JBoss/RESTEasy artifacts
with %{build_v1_api} but leaves Jackson JAX-RS providers unguarded; update the
spec to wrap the jackson-jaxrs-base and jackson-jaxrs-json-provider
BuildRequires entries (the same artifacts added to the v1-api-deps profile in
base/common/pom.xml) inside the %{build_v1_api} conditional alongside the
existing resteasy lines, and also update the %post cleanup logic that removes v1
API jars to include wildcard matches for jackson-jaxrs-* so stale
jackson-jaxrs-base and jackson-jaxrs-json-provider jars are removed in dropped
builds (apply the same changes to the other mentioned blocks at lines
corresponding to 618-625, 651-659, 1917-1933).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71c0f185-e932-4dd1-a97e-ffdc21dd8244
📒 Files selected for processing (16)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.javabase/common/pom.xmlbase/javadoc/CMakeLists.txtbase/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.javabase/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.javabase/pom.xmlbase/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.javabase/server/pom.xmlbase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.javabase/server/src/main/resources/build.propertiesbase/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.javabase/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.javabuild.shdocs/changes/v11.10.0/Packaging-Changes.adocdocs/changes/v11.10.0/Server-Changes.adocpki.spec
fmarco76
force-pushed
the
RestV1LifeCycle
branch
from
3bee350 to
5b7ae02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.java (1)
52-60:⚠️ Potential issue | 🟠 MajorAdd explicit
@OPTIONShandler to ensure disabled v1 returns 410 on all requests.When no
@OPTIONSmethod is defined, JAX-RS automatically returns 200 OK with an "Allow" header for OPTIONS requests instead of the custom 410 response. This breaks the "all v1 endpoints return HTTP 410 Gone" contract and affects CORS preflights.Proposed fix
+import javax.ws.rs.OPTIONS; `@GET` `@POST` `@PUT` `@DELETE` `@PATCH` `@Path`("{path:.*}") public Response handleAll(`@PathParam`("path") String path) { return createErrorResponse(); } + + `@OPTIONS` + `@Path`("{path:.*}") + public Response handleOptions(`@PathParam`("path") String path) { + return createErrorResponse(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.java` around lines 52 - 60, The V1ApiDisabledResource currently lacks an explicit `@OPTIONS` handler so JAX-RS responds to OPTIONS with 200 and an Allow header; add an `@OPTIONS-annotated` method (e.g., public Response handleOptions(`@PathParam`("path") String path)) with the same `@Path`("{path:.*}") that simply returns createErrorResponse(), ensuring all HTTP methods including OPTIONS return the 410 Gone behavior; reference the class V1ApiDisabledResource and the existing createErrorResponse() helper to implement the new handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pki.spec`:
- Around line 283-291: The v1-api-deps profile currently uses activeByDefault
only, so jackson-jaxrs-base and jackson-jaxrs-json-provider remain included even
when v1.api.status=dropped; update the v1-api-deps profile activation to gate on
the v1.api.status property so the profile is disabled when v1.api.status is set
to "dropped" (i.e., modify the <activation> for profile id v1-api-deps in
base/common/pom.xml to include a property condition referencing v1.api.status
that prevents activation for the value "dropped", ensuring jackson-jaxrs-base
and jackson-jaxrs-json-provider are excluded in dropped builds).
---
Duplicate comments:
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.java`:
- Around line 52-60: The V1ApiDisabledResource currently lacks an explicit
`@OPTIONS` handler so JAX-RS responds to OPTIONS with 200 and an Allow header; add
an `@OPTIONS-annotated` method (e.g., public Response
handleOptions(`@PathParam`("path") String path)) with the same `@Path`("{path:.*}")
that simply returns createErrorResponse(), ensuring all HTTP methods including
OPTIONS return the 410 Gone behavior; reference the class V1ApiDisabledResource
and the existing createErrorResponse() helper to implement the new handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 291e5b24-e986-4627-ba8f-3f3ea8be4ecf
📒 Files selected for processing (17)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.javabase/common/pom.xmlbase/javadoc/CMakeLists.txtbase/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.javabase/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.javabase/pom.xmlbase/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.javabase/server/pom.xmlbase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.javabase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.javabase/server/src/main/resources/build.propertiesbase/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.javabase/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.javabuild.shdocs/changes/v11.10.0/Packaging-Changes.adocdocs/changes/v11.10.0/Server-Changes.adocpki.spec
✅ Files skipped from review due to trivial changes (3)
- base/server/src/main/resources/build.properties
- docs/changes/v11.10.0/Server-Changes.adoc
- docs/changes/v11.10.0/Packaging-Changes.adoc
🚧 Files skipped from review as they are similar to previous changes (6)
- base/server/pom.xml
- base/pom.xml
- base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.java
- base/common/pom.xml
- base/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.java
- base/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.java
fmarco76
force-pushed
the
RestV1LifeCycle
branch
from
5b7ae02 to
51b6b95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java (1)
61-83:⚠️ Potential issue | 🟠 MajorDon't fail open on unsupported
v1.api.statusvalues.Line 63 takes the resolved runtime/default status verbatim, and anything other than
disabledordeprecatedfalls through to normal registration.-Dv1.api.status=droppedor a typo therefore re-enables v1 instead of blocking it. Normalize the resolved value first, mapdroppedtodisabled, and fail fast on anything else.Proposed fix
public static boolean checkV1ApiStatus(String subsystemName, Set<Class<?>> classes, org.slf4j.Logger subsystemLogger) { - // Use build-time default unless overridden by system property - String v1ApiStatus = System.getProperty("v1.api.status", getDefaultV1ApiStatus()); + // Use build-time default unless overridden by system property + String v1ApiStatus = normalizeV1ApiStatus( + System.getProperty("v1.api.status", getDefaultV1ApiStatus())); if ("disabled".equals(v1ApiStatus)) { subsystemLogger.warn("======================================================================"); subsystemLogger.warn(subsystemName + " v1 REST API has been DISABLED."); subsystemLogger.warn("All v1 endpoints will return HTTP 410 Gone."); @@ return false; // continue normally } + + private static String normalizeV1ApiStatus(String v1ApiStatus) { + switch (v1ApiStatus) { + case "enabled": + case "deprecated": + case "disabled": + return v1ApiStatus; + case "dropped": + return "disabled"; + default: + throw new IllegalArgumentException("Invalid v1.api.status: " + v1ApiStatus); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java` around lines 61 - 83, The method checkV1ApiStatus currently trusts the raw System.getProperty("v1.api.status") value; normalize it (trim and toLowerCase), map the legacy value "dropped" to "disabled", then switch on the normalized v1ApiStatus: if "disabled" register V1ApiDisabledResource in classes and return true; if "deprecated" log the deprecation warnings and continue; if it's any other value log an error via subsystemLogger (include the resolved value and allowed values) and fail fast by throwing an IllegalArgumentException so startup doesn't accidentally enable v1. Use the existing symbols checkV1ApiStatus, getDefaultV1ApiStatus, v1ApiStatus, V1ApiDisabledResource, subsystemLogger and classes to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/javadoc/CMakeLists.txt`:
- Around line 67-90: The current dropped-mode branch sets exclude_packages but
only omits the shared rest.v1 and tps roots; update the exclude_packages list
(the variable named exclude_packages in the CMake snippet) to include each
subsystem-specific v1 package root (e.g. add org.dogtagpki.server.kra.rest.v1
for KRA and any other subsystem packages that follow the *.rest.v1 pattern) so
javadoc traversal skips all subsystem v1 packages when V1_API_STATUS is
"dropped"; keep v1_subpackages, jaxrs_classpath and v1_javadoc_opts behavior the
same.
In `@pki.spec`:
- Around line 164-173: The spec currently only special-cases "dropped" but
doesn't validate arbitrary values for the macro v1_api_status, so rpmbuild
--define "v1_api_status foo" will silently produce a wrong build; add a
spec-time guard that checks %{v1_api_status} against the allowed set (enabled,
deprecated, disabled, dropped) before computing build_v1_api and emit a fatal
%error for any other value. Locate the v1_api_status macro and the build_v1_api
conditional (symbols: v1_api_status and build_v1_api) and insert a short
%if/%else block that tests the macro value and calls %error with a clear message
if it’s not one of the four allowed values, then proceed to set build_v1_api as
before.
---
Duplicate comments:
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java`:
- Around line 61-83: The method checkV1ApiStatus currently trusts the raw
System.getProperty("v1.api.status") value; normalize it (trim and toLowerCase),
map the legacy value "dropped" to "disabled", then switch on the normalized
v1ApiStatus: if "disabled" register V1ApiDisabledResource in classes and return
true; if "deprecated" log the deprecation warnings and continue; if it's any
other value log an error via subsystemLogger (include the resolved value and
allowed values) and fail fast by throwing an IllegalArgumentException so startup
doesn't accidentally enable v1. Use the existing symbols checkV1ApiStatus,
getDefaultV1ApiStatus, v1ApiStatus, V1ApiDisabledResource, subsystemLogger and
classes to locate and implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 244435a3-1cea-43ca-a71d-9b4d29e766c1
📒 Files selected for processing (17)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.javabase/common/pom.xmlbase/javadoc/CMakeLists.txtbase/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.javabase/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.javabase/pom.xmlbase/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.javabase/server/pom.xmlbase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.javabase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.javabase/server/src/main/resources/build.propertiesbase/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.javabase/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.javabuild.shdocs/changes/v11.10.0/Packaging-Changes.adocdocs/changes/v11.10.0/Server-Changes.adocpki.spec
✅ Files skipped from review due to trivial changes (5)
- base/server/src/main/resources/build.properties
- base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.java
- base/server/pom.xml
- docs/changes/v11.10.0/Packaging-Changes.adoc
- docs/changes/v11.10.0/Server-Changes.adoc
🚧 Files skipped from review as they are similar to previous changes (4)
- base/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.java
- base/common/pom.xml
- base/pom.xml
- base/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.java
fmarco76
force-pushed
the
RestV1LifeCycle
branch
from
51b6b95 to
bdad48c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pki.spec (1)
164-173:⚠️ Potential issue | 🟠 MajorReject unknown
%{v1_api_status}values in the spec itself.Right now every value except
droppedbecomesbuild_v1_api=1, so directrpmbuild --define "v1_api_status foo"bypasses the wrapper validation and takes the wrong spec-time path. Fail fast here before derivingbuild_v1_api.Example fix
%{!?v1_api_status:%global v1_api_status enabled} + +%if "%{v1_api_status}" != "enabled" && "%{v1_api_status}" != "deprecated" && "%{v1_api_status}" != "disabled" && "%{v1_api_status}" != "dropped" +%error Invalid v1_api_status '%{v1_api_status}'. Valid values: enabled, deprecated, disabled, dropped. +%endif # Build v1 API unless status is "dropped" %if "%{v1_api_status}" == "dropped" %global build_v1_api 0 %else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pki.spec` around lines 164 - 173, The spec currently accepts any v1_api_status and only treats "dropped" specially; add validation for acceptable values ("enabled", "deprecated", "disabled", "dropped") before computing build_v1_api and fail fast for any other value by using a %if/%elseif chain that calls %error for unknown values; locate the existing v1_api_status and build_v1_api logic and replace it with a validation block that checks %{v1_api_status} against those four literals and then sets %global build_v1_api accordingly (1 for enabled/deprecated/disabled, 0 for dropped).base/javadoc/CMakeLists.txt (1)
73-78:⚠️ Potential issue | 🟠 MajorTighten dropped-mode filtering to the actual v1 package roots.
This list is both too broad and too narrow: excluding
org.dogtagpki.server.tpsdrops the whole TPS namespace, while subsystem-specific*.rest.v1packages are still eligible through the recursive subpackage walk. Please replace the coarse exclusions with the concrete*.rest.v1package roots so dropped builds hide only v1 docs.Example direction
set(exclude_packages "com.netscape.certsrv" "com.netscape.cms.servlet" "org.dogtagpki.server.rest.v1" - "org.dogtagpki.server.tps" + "org.dogtagpki.server.ca.rest.v1" + "org.dogtagpki.server.kra.rest.v1" + "org.dogtagpki.server.ocsp.rest.v1" + "org.dogtagpki.server.tks.rest.v1" + "org.dogtagpki.server.tps.rest.v1" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/javadoc/CMakeLists.txt` around lines 73 - 78, The current exclude_packages list is too coarse—remove the broad "org.dogtagpki.server.tps" entry and replace the recursive/ambiguous exclusion with explicit v1 roots only (e.g., keep or add concrete package roots such as "org.dogtagpki.server.rest.v1" and any other subsystem-specific "*.rest.v1" package roots needed) so that dropped-mode filtering only hides v1 docs; update the set(exclude_packages ...) block accordingly to list only the exact rest.v1 package roots rather than entire subsystems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.java`:
- Around line 53-60: The handleAll method in V1ApiDisabledResource is
incorrectly annotated with multiple HTTP method annotations; split it into
separate resource methods—one each for GET, POST, PUT, DELETE, OPTIONS, and
PATCH—each annotated with the single corresponding HTTP annotation and the same
`@Path`("{path:.*}") and `@PathParam`("path") signature, and have each new method
delegate to the existing createErrorResponse() to build the Response; ensure
method names are distinct (e.g., handleGet, handlePost, handlePut, handleDelete,
handleOptions, handlePatch) and preserve the original parameter and return
types.
---
Duplicate comments:
In `@base/javadoc/CMakeLists.txt`:
- Around line 73-78: The current exclude_packages list is too coarse—remove the
broad "org.dogtagpki.server.tps" entry and replace the recursive/ambiguous
exclusion with explicit v1 roots only (e.g., keep or add concrete package roots
such as "org.dogtagpki.server.rest.v1" and any other subsystem-specific
"*.rest.v1" package roots needed) so that dropped-mode filtering only hides v1
docs; update the set(exclude_packages ...) block accordingly to list only the
exact rest.v1 package roots rather than entire subsystems.
In `@pki.spec`:
- Around line 164-173: The spec currently accepts any v1_api_status and only
treats "dropped" specially; add validation for acceptable values ("enabled",
"deprecated", "disabled", "dropped") before computing build_v1_api and fail fast
for any other value by using a %if/%elseif chain that calls %error for unknown
values; locate the existing v1_api_status and build_v1_api logic and replace it
with a validation block that checks %{v1_api_status} against those four literals
and then sets %global build_v1_api accordingly (1 for
enabled/deprecated/disabled, 0 for dropped).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb035973-f00e-4c6a-ba9a-31f9afda0b42
📒 Files selected for processing (17)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.javabase/common/pom.xmlbase/javadoc/CMakeLists.txtbase/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.javabase/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.javabase/pom.xmlbase/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.javabase/server/pom.xmlbase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.javabase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.javabase/server/src/main/resources/build.propertiesbase/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.javabase/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.javabuild.shdocs/changes/v11.10.0/Packaging-Changes.adocdocs/changes/v11.10.0/Server-Changes.adocpki.spec
✅ Files skipped from review due to trivial changes (4)
- base/server/src/main/resources/build.properties
- base/server/pom.xml
- docs/changes/v11.10.0/Server-Changes.adoc
- docs/changes/v11.10.0/Packaging-Changes.adoc
🚧 Files skipped from review as they are similar to previous changes (9)
- base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.java
- base/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.java
- base/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.java
- base/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.java
- base/common/pom.xml
- base/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.java
- base/pom.xml
- base/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.java
- base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java
|
Contributor
|
Directly from Cursor, but filtered to what might be relevant only:
I'm proceeding to test it locally |
Contributor
|
tested loacally with dropped instead I had some problems, build fails with error messages about dependencies Building pki-11.10.0-alpha1
3.178 RPM sources:
3.179 /root/pki/build/SOURCES/pki-11.10.0-alpha1.tar.gz
3.191 RPM spec:
3.192 /root/pki/build/SPECS/pki.spec
3.662 SRPM package:
3.662 /root/pki/build/SRPMS/pki-11.10.0~alpha1-1.fc43.src.rpm
3.744 Installing /root/pki/build/SRPMS/pki-11.10.0~alpha1-1.fc43.src.rpm
3.744 setting SOURCE_DATE_EPOCH=1520294400
3.756 error: Failed build dependencies:
3.756 mvn(com.fasterxml.jackson.jaxrs:jackson-jaxrs-base) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.logging:jboss-logging) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.resteasy:resteasy-client) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.resteasy:resteasy-jackson2-provider) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.resteasy:resteasy-jaxrs) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.resteasy:resteasy-servlet-initializer) is needed by pki-11.10.0~alpha1-1.fc43.x86_64
3.756 mvn(org.jboss.spec.javax.ws.rs:jboss-jaxrs-api_2.0_spec) is needed by pki-11.10.0~alpha1-1.fc43.x86_64 |
Introduce a new --v1-api-status flag to build.sh that provides granular
control over the v1 REST API lifecycle, enabling smooth migration from
v1 to v2 APIs.
Four status levels are supported:
1. enabled: v1 API fully functional, backward compatible
2. deprecated: v1 functional but logs migration warnings at startup
3. disabled (default): v1 compiled but returns HTTP 410 Gone at runtime
4. dropped: v1 code completely excluded from compilation
Build-time Integration:
- build.sh accepts --v1-api-status={enabled|deprecated|disabled|dropped}
- Default is disabled, enforcing migration to v2 by default
- Passes value to CMake via -DV1_API_STATUS and to RPM via --define
- Default value embedded in build.properties resource file
- Maven filtering injects value into JAR at compile time
Status: dropped (most significant changes):
- Maven v1-api-dropped profile excludes v1 source files via pluginManagement
- Maven v1-api-deps profile deactivated automatically via property activation
- JAX-RS/RESTEasy BuildRequires and Requires skipped in pki.spec
- JAX-RS/RESTEasy JARs not installed or transformed
- CMake javadoc excludes v1 REST packages only (not entire subsystems)
- RPM %post script removes old JAX-RS JARs from existing instances
- Binary RPMs ~2-3MB smaller without v1 code and dependencies
Status: disabled (default, clean error handling):
- V1ApiDisabledResource catch-all JAX-RS resource created
- Returns HTTP 410 Gone with JSON error instead of stack trace
- Separate methods for each HTTP verb per JAX-RS specification
- All v1 Application classes register only V1ApiDisabledResource
- Clean user experience: {"error": "v1 REST API has been disabled", ...}
- Standard headers: Deprecation, Sunset, Link (RFC 8594, IETF draft)
Status: deprecated (migration warning):
- All v1 Application classes log prominent warning on startup
- Full functionality maintained for transition period
- Warnings visible in systemd journal and tomcat logs
- V1ApiDeprecationFilter adds standard headers to all responses
- Headers: Deprecation: true, Link to v2 API documentation
Runtime Behavior:
- Build-time status becomes default runtime behavior
- Administrators can override via -Dv1.api.status system property
- All v1 Application classes (PKI, CA, KRA, OCSP, TKS, TPS) updated
- Default behavior read from build.properties at runtime
- V1ApiStatusHelper centralizes status checking logic
- Runtime values normalized (trimmed, lowercased)
- Invalid values logged and fall back to disabled
- Runtime "dropped" mapped to "disabled" (cannot drop code dynamically)
Implementation Details:
- base/pom.xml: v1-api-deps and v1-api-dropped Maven profiles
- base/common/pom.xml: Property-based profile activation (!dropped)
- base/server/pom.xml: Resource filtering for build.properties
- base/server/src/main/resources/build.properties: New configuration file
- base/server/src/main/java/.../V1ApiDisabledResource.java: New error handler
- base/server/src/main/java/.../V1ApiDeprecationFilter.java: Response filter
- base/server/src/main/java/.../V1ApiStatusHelper.java: Shared utility
- base/javadoc/CMakeLists.txt: Conditional package exclusion
- pki.spec: Conditional dependencies, validation, and %post JAR cleanup
- build.sh: Flag parsing and CMake/RPM integration
HTTP Headers (Standards Compliance):
- Deprecation header (IETF draft-dalal-deprecation-header)
- Sunset header (RFC 8594) for disabled state
- Link header (RFC 8288) pointing to v2 API documentation
- Programmatic detection by API clients and tooling
RPM Build Notes:
- When using build.sh: ./build.sh --v1-api-status=<status>
- When using rpmbuild: rpmbuild --define "v1_api_status <status>"
- SRPMs do not embed macro values - must re-pass --define on rebuild
- Spec file validates v1_api_status value - fail fast on invalid input
Migration Path:
1. Default behavior (disabled) enforces v2 migration immediately
2. Override with --v1-api-status=deprecated for gradual migration
3. Override with --v1-api-status=enabled for backward compatibility
4. Deploy with --v1-api-status=dropped to remove v1 code permanently
Testing:
- Verified compilation with all four status values
- Verified runtime behavior (enabled, deprecated, disabled)
- Verified v1 code exclusion and JAR removal (dropped)
- Verified HTTP 410 Gone responses (disabled)
- Verified javadoc generation without v1 packages (dropped)
- Verified standard deprecation headers
- Verified input validation and normalization
Documentation: See docs/changes/v11.10.0/Packaging-Changes.adoc and Server-Changes.adoc
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
fmarco76
force-pushed
the
RestV1LifeCycle
branch
from
bdad48c to
bb01110
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/javadoc/CMakeLists.txt`:
- Around line 71-93: The dropped build is clearing v1_subpackages which removes
the entire org.dogtagpki tree; restore v1_subpackages to include "org.dogtagpki"
(and keep "com.netscape.certsrv" if intended) instead of setting it to empty,
and leave exclude_packages containing only the v1 REST packages
("org.dogtagpki.server.rest.v1", "org.dogtagpki.server.ca.rest.v1", etc.),
keeping jaxrs_classpath and v1_javadoc_opts behavior unchanged so only *.rest.v1
packages are excluded while non-v1 Dogtag Javadoc is still generated.
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java`:
- Around line 80-98: The current code silently treats any invalid runtime
override as "disabled"; change checkV1ApiStatus to compute the normalized
build-time default (String buildDefault =
normalizeStatus(getDefaultV1ApiStatus())) and after normalizing the runtime
rawStatus (String v1ApiStatus = normalizeStatus(rawStatus)) detect when the
runtime value normalized to "disabled" but the buildDefault is not "disabled"
(indicating an invalid override) — in that case log a warning and set
v1ApiStatus = buildDefault instead of forcing "disabled"; alternatively you may
throw an IllegalArgumentException to fail fast, but do not silently convert bad
input into an outage. Ensure you reference rawStatus, v1ApiStatus,
normalizeStatus, and getDefaultV1ApiStatus in the change.
In `@pki.spec`:
- Around line 164-168: The spec macro default currently sets v1_api_status to
"disabled" which conflicts with the PR's canonical default of enabled; update
the macro definition (the %{!?v1_api_status:%global v1_api_status ...} line) to
set the default to "enabled" (or otherwise match the project's canonical
default) so that direct rpmbuild/SRPM rebuilds inherit the same v1_api_status as
normal builds; ensure you only change the value in the v1_api_status macro and
leave the surrounding comment text intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c72c7231-14f4-4d21-95c4-0cf8afffb61f
📒 Files selected for processing (18)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.javabase/common/pom.xmlbase/javadoc/CMakeLists.txtbase/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.javabase/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.javabase/pom.xmlbase/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.javabase/server/pom.xmlbase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDeprecationFilter.javabase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiDisabledResource.javabase/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.javabase/server/src/main/resources/build.propertiesbase/tks/src/main/java/org/dogtagpki/server/tks/rest/v1/TKSApplication.javabase/tps/src/main/java/org/dogtagpki/server/tps/rest/v1/TPSApplication.javabuild.shdocs/changes/v11.10.0/Packaging-Changes.adocdocs/changes/v11.10.0/Server-Changes.adocpki.spec
✅ Files skipped from review due to trivial changes (3)
- base/server/src/main/resources/build.properties
- docs/changes/v11.10.0/Server-Changes.adoc
- docs/changes/v11.10.0/Packaging-Changes.adoc
🚧 Files skipped from review as they are similar to previous changes (7)
- base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/CAApplication.java
- base/pom.xml
- base/kra/src/main/java/org/dogtagpki/server/kra/rest/v1/KRAApplication.java
- base/server/pom.xml
- base/ocsp/src/main/java/org/dogtagpki/server/ocsp/rest/v1/OCSPApplication.java
- base/server-webapp/src/main/java/org/dogtagpki/server/rest/v1/PKIApplication.java
- build.sh
Comment on lines
+71
to
+93
| set(v1_subpackages "") | ||
| # Exclude v1-specific packages (both client and server) | ||
| # Only exclude v1 REST packages, not entire subsystem packages | ||
| set(exclude_packages | ||
| "com.netscape.certsrv" | ||
| "com.netscape.cms.servlet" | ||
| "org.dogtagpki.server.rest.v1" | ||
| "org.dogtagpki.server.ca.rest.v1" | ||
| "org.dogtagpki.server.kra.rest.v1" | ||
| "org.dogtagpki.server.ocsp.rest.v1" | ||
| "org.dogtagpki.server.tks.rest.v1" | ||
| "org.dogtagpki.server.tps.rest.v1" | ||
| ) | ||
| # Don't include JAX-RS jars - they're not available when v1 is dropped | ||
| # and javadoc will skip type checking for excluded packages | ||
| set(jaxrs_classpath "") | ||
| # Ignore source errors in excluded packages (transformed code references jakarta.ws.rs but jars have javax.ws.rs) | ||
| set(v1_javadoc_opts "--ignore-source-errors") | ||
| else() | ||
| # v1 enabled/deprecated/disabled: include all packages | ||
| message(STATUS "v1 API ${V1_API_STATUS}: including all packages in javadoc") | ||
| set(v1_subpackages "com.netscape.certsrv" "org.dogtagpki") | ||
| set(exclude_packages "") |
There was a problem hiding this comment.
Dropped mode is removing all org.dogtagpki Javadoc, not just v1 REST docs.
Line 71 clears v1_subpackages, so the Javadoc target stops traversing any org.dogtagpki.* packages in dropped builds. That makes the explicit EXCLUDE list below ineffective for preserving non-v1 Dogtag docs. Keep org.dogtagpki in SUBPACKAGES and let EXCLUDE strip only the *.rest.v1 packages.
Proposed fix
- # Don't include com.netscape.certsrv or org.dogtagpki (which are mostly v1 Resource interfaces)
- set(v1_subpackages "")
+ # Keep non-v1 Dogtag packages in Javadoc and exclude only v1 REST packages explicitly.
+ set(v1_subpackages "org.dogtagpki")Based on learnings, the broad com.netscape.certsrv omission is intentional for v1-only JAX-RS interfaces; the regression here is dropping the entire org.dogtagpki tree from SUBPACKAGES.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/javadoc/CMakeLists.txt` around lines 71 - 93, The dropped build is
clearing v1_subpackages which removes the entire org.dogtagpki tree; restore
v1_subpackages to include "org.dogtagpki" (and keep "com.netscape.certsrv" if
intended) instead of setting it to empty, and leave exclude_packages containing
only the v1 REST packages ("org.dogtagpki.server.rest.v1",
"org.dogtagpki.server.ca.rest.v1", etc.), keeping jaxrs_classpath and
v1_javadoc_opts behavior unchanged so only *.rest.v1 packages are excluded while
non-v1 Dogtag Javadoc is still generated.
Comment on lines
+80
to
+98
| // Invalid value - log warning and fall back to default | ||
| logger.warn("Invalid v1.api.status value: '" + rawStatus + "'. " + | ||
| "Valid values are: enabled, deprecated, disabled, dropped. " + | ||
| "Falling back to 'disabled'."); | ||
| return "disabled"; | ||
| } | ||
|
|
||
| /** | ||
| * Check v1 API status and handle disabled/deprecated states. | ||
| * | ||
| * @param subsystemName The name of the subsystem (e.g., "CA", "KRA", "PKI") | ||
| * @param classes The set of classes to modify if disabled or deprecated | ||
| * @param subsystemLogger The logger to use for warnings | ||
| * @return true if the Application constructor should return early (disabled state), false otherwise | ||
| */ | ||
| public static boolean checkV1ApiStatus(String subsystemName, Set<Class<?>> classes, org.slf4j.Logger subsystemLogger) { | ||
| // Use build-time default unless overridden by system property | ||
| String rawStatus = System.getProperty("v1.api.status", getDefaultV1ApiStatus()); | ||
| String v1ApiStatus = normalizeStatus(rawStatus); |
There was a problem hiding this comment.
Invalid runtime overrides should not silently force disabled.
Lines 80-84 hard-code the fallback to disabled. That means a typo like -Dv1.api.status=deprected takes every v1 application offline even when the build-time default is enabled or deprecated. Fall back to the normalized build default here, or fail fast during startup, but don’t silently convert bad input into an outage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/server/src/main/java/org/dogtagpki/server/rest/v1/V1ApiStatusHelper.java`
around lines 80 - 98, The current code silently treats any invalid runtime
override as "disabled"; change checkV1ApiStatus to compute the normalized
build-time default (String buildDefault =
normalizeStatus(getDefaultV1ApiStatus())) and after normalizing the runtime
rawStatus (String v1ApiStatus = normalizeStatus(rawStatus)) detect when the
runtime value normalized to "disabled" but the buildDefault is not "disabled"
(indicating an invalid override) — in that case log a warning and set
v1ApiStatus = buildDefault instead of forcing "disabled"; alternatively you may
throw an IllegalArgumentException to fail fast, but do not silently convert bad
input into an outage. Ensure you reference rawStatus, v1ApiStatus,
normalizeStatus, and getDefaultV1ApiStatus in the change.
Comment on lines
+164
to
+168
| # V1 API status: enabled, deprecated, disabled (default), dropped | ||
| # When building via build.sh, use: ./build.sh --v1-api-status=<status> | ||
| # When building via rpmbuild directly, use: rpmbuild --define "v1_api_status <status>" | ||
| # Note: SRPMs do not embed macro values - you must re-pass --define during rebuild | ||
| %{!?v1_api_status:%global v1_api_status disabled} |
There was a problem hiding this comment.
The spec default now flips v1 to disabled-by-default.
Line 168 makes direct rpmbuild/SRPM rebuilds default to disabled, so this build path no longer matches the PR’s stated default-enabled behavior unless every caller remembers to pass --define "v1_api_status ...". The spec default should match the canonical build default.
Proposed fix
-%{!?v1_api_status:%global v1_api_status disabled}
+%{!?v1_api_status:%global v1_api_status enabled}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # V1 API status: enabled, deprecated, disabled (default), dropped | |
| # When building via build.sh, use: ./build.sh --v1-api-status=<status> | |
| # When building via rpmbuild directly, use: rpmbuild --define "v1_api_status <status>" | |
| # Note: SRPMs do not embed macro values - you must re-pass --define during rebuild | |
| %{!?v1_api_status:%global v1_api_status disabled} | |
| # V1 API status: enabled, deprecated, disabled (default), dropped | |
| # When building via build.sh, use: ./build.sh --v1-api-status=<status> | |
| # When building via rpmbuild directly, use: rpmbuild --define "v1_api_status <status>" | |
| # Note: SRPMs do not embed macro values - you must re-pass --define during rebuild | |
| %{!?v1_api_status:%global v1_api_status enabled} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pki.spec` around lines 164 - 168, The spec macro default currently sets
v1_api_status to "disabled" which conflicts with the PR's canonical default of
enabled; update the macro definition (the %{!?v1_api_status:%global
v1_api_status ...} line) to set the default to "enabled" (or otherwise match the
project's canonical default) so that direct rpmbuild/SRPM rebuilds inherit the
same v1_api_status as normal builds; ensure you only change the value in the
v1_api_status macro and leave the surrounding comment text intact.
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
2 participants
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.
Introduce a new --v1-api-status flag to build.sh that provides granular control over the v1 REST API lifecycle, enabling smooth migration from v1 to v2 APIs.
Four status levels are supported:
Build-time Integration:
Status: dropped (most significant changes):
Status: disabled (clean error handling):
Status: deprecated (migration warning):
Runtime Behavior:
Implementation Details:
Migration Path:
Testing:
Documentation: See docs/changes/v11.10.0/Packaging-Changes.adoc and Server-Changes.adoc
Summary by CodeRabbit
New Features
Behavior Changes
Documentation