[rest] Corrections in the annotations for several REST api calls

[jsjames]

• POST /states/{connection Id} needs to be annotated to accept JSON
• GET /persistence/strategysuggestions will return either PersistenceStrategy OR PersistenceCronStrategy
• PUT /services/{serviceId}/config accepts an object as body and responds with an object, not a string
• GET /services/{serviceId}/config returns an object, not a string

No need to use PersistenceStrategyDTO class since we can directly use PersistenceStrategy - so deleted unecessary file.

[mherwege]

I am not sure it is a good idea to remove the DTO. The suggestions are defined in an interface, implemented by individual persistence services. The interfaces don't define fields for these, only methods that return them. GSON serialization will only serialize fields.
Depending on the exact implementation in the specific persistence service, this may be fine, but it is not guaranteed. Did you try with multiple persistence services?
I was hit with this recently when trying to fix the getItemInfo persistence endpoint, needed to add a DTO for that reason.

[jsjames]

The PersistenceStrategyDTO was not used at all in the code, it was only used in the swagger annotations to generate the openapi spec - it is less error prone to just use the actual structure. So removing has no impact on any of the code. Also, many other swagger annotations just use the object as well.

Are there other types of Persistence*Strategies we need to make sure are included?

Here are the relevant sections of the generated openapi spec given the change here:

        "/persistence/strategysuggestions": {
            "get": {
                "tags": [
                    "persistence"
                ],
                "summary": "Gets a persistence service suggested strategies.",
                "operationId": "getPersistenceServiceStrategySuggestions",
                "parameters": [
                    {
                        "name": "serviceId",
                        "in": "query",
                        "description": "Id of the persistence service.",
                        "schema": {
                            "type": "string"
                        }
                    }
                ],
                "responses": {
                    "200": {
                        "description": "OK",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "uniqueItems": true,
                                    "type": "array",
                                    "items": {
                                        "type": "string",
                                        "oneOf": [
                                            {
                                                "$ref": "#/components/schemas/PersistenceStrategy"
                                            },
                                            {
                                                "$ref": "#/components/schemas/PersistenceCronStrategy"
                                            }
                                        ]
                                    }
                                }
                            }
                        }
                    },
...

           "PersistenceStrategy": {
                "type": "object",
                "properties": {
                    "name": {
                        "type": "string"
                    }
                }

...

            "PersistenceCronStrategy": {
                "type": "object",
                "properties": {
                    "name": {
                        "type": "string"
                    },
                    "cronExpression": {
                        "type": "string"
                    }
                }
            },

[mherwege]

All good. If it was only used for swagger, it can be removed. But it actually works through because of some luck.

As pointed out the information comes from a call to an interface. That interface only defines methods, not fields. If the persistence service does not have fields backing the response to these method calls, GSON will only serialize partial information.
Your change does not change the current behaviour, but I would almost argue that we need a DTO to deliver the information and we need to populate the fields in it from the method calls. That makes serialization save. The DTO can just be a record that is embedded in the class by the way (like I have done for the getItemInfo endpoint). That statement is beyond the scope of the change you make, and as it works right now, not something that you need to do.

So this change LGTM.

[Copilot]

Pull request overview

This PR corrects REST API OpenAPI annotations for several endpoints to better reflect the actual request/response types. The changes include adding a @Consumes annotation to the SSE states update endpoint, updating configuration endpoints to use object schemas instead of string schemas, and replacing a custom PersistenceStrategyDTO with direct references to PersistenceStrategy/PersistenceCronStrategy classes in the swagger schema annotation.

Changes:

• SSE endpoint POST /states/{connectionId} now correctly declares it consumes application/json
• Config service endpoints corrected from String to object schema type for responses and request body
• PersistenceStrategyDTO deleted; PersistenceResource updated to reference domain classes in OpenAPI oneOf schema annotation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
PersistenceStrategyDTO.java	Deleted DTO class; no remaining usages found in the codebase
SseResource.java	Added @Consumes(MediaType.APPLICATION_JSON) to the POST /states/{connectionId} endpoint
ConfigurableServiceResource.java	Corrected schema type from String.class to type = "object" in GET, PUT, and DELETE config endpoint annotations; added proper requestBody annotation to PUT
PersistenceResource.java	Replaced PersistenceStrategyDTO reference with oneOf = { PersistenceStrategy.class, PersistenceCronStrategy.class } in the strategy suggestions endpoint annotation

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The PR description states "PUT /states/{connection Id} needs to be annotated to accept JSON", but the actual HTTP method used by this endpoint is @POST (not @PUT). The code change itself is correct (adding @Consumes(MediaType.APPLICATION_JSON) to the POST endpoint), but the PR description contains an inaccurate method name.

[jsjames]

fixed comment

[Copilot]

The @Schema(oneOf = { PersistenceStrategy.class, PersistenceCronStrategy.class }) annotation references domain model classes that lack @Schema annotations and whose fields are all private. OpenAPI schema introspection (via Swagger) will not be able to generate a meaningful schema for these classes — the resulting schema descriptions will be incomplete or empty.

The existing PersistenceCronStrategyDTO (with annotated name and cronExpression public fields) is the proper DTO to use for PersistenceCronStrategy. Similarly, the deleted PersistenceStrategyDTO (or an equivalent DTO class) with proper @Schema annotation is what should be referenced for PersistenceStrategy.

Using the domain classes PersistenceStrategy and PersistenceCronStrategy directly in the annotation is incorrect for OpenAPI documentation purposes. Consider either keeping/restoring the DTOs or adding proper @Schema annotations and Jackson/Swagger visibility configuration to the domain classes.

[jsjames]

Swagger does look at properties (getName for instance) in generating the spec, so even though the fields are private, the correct openapi spec is generated. As such, it is recommended to keep the code straight forward and use the classes as is.

[jsjames]

All good. If it was only used for swagger, it can be removed. But it actually works through because of some luck.

As pointed out the information comes from a call to an interface. That interface only defines methods, not fields. If the persistence service does not have fields backing the response to these method calls, GSON will only serialize partial information. Your change does not change the current behaviour, but I would almost argue that we need a DTO to deliver the information and we need to populate the fields in it from the method calls. That makes serialization save. The DTO can just be a record that is embedded in the class by the way (like I have done for the getItemInfo endpoint). That statement is beyond the scope of the change you make, and as it works right now, not something that you need to do.

So this change LGTM.

Copilot agrees with you that the DTO should be maintained :). I'll add it back in.

[jsjames]

Reference

@mherwege - could use your help here - the current PersistenceStrategyDTO does not have a name field, so I believe we need to add that, correct? Or, is it correct that only the PersistenceCronStrategy has a name field?

Are the rest of the fields accurate?

    public String type;
    public String configuration;

Also, are all the fields are mandatory or provided for each both DTOs?

[mherwege]

Reference

@mherwege - could use your help here - the current PersistenceStrategyDTO does not have a name field, so I believe we need to add that, correct? Or, is it correct that only the PersistenceCronStrategy has a name field?

Are the rest of the fields accurate?

    public String type;
    public String configuration;

Also, are all the fields are mandatory or provided for each both DTOs?

PersistenceStrategy only has a name field. PersistenceCronStrategy extends PersistenceStrategy and has a name and cronExpression field. All fields are required. And these fields are also what is currently returned (I did a quick test for rrd4j). It looks like to DTO is totally wrong. And there is never an assignment to the DTO fields.
I would change the DTO to work with the name and cronExpression fields. cronExpression in this case could be nullable. I also think before returning anything, the DTO fields should be assigned from method calls to avoid any potential future GSON serialization issues with persistence services not having fields for these.

[jsjames]

Both GSON and swagger look for get* properties in serialization/openapi spec generation, so, even though both fields are private in their class, there should be no issue with serialization. In looking at the code again, I think we are making more work to use the DTO classes vs. just using the regular classes. So, I recommend we keep things as I submitted in this PR.

[mherwege]

Both GSON and swagger look for get* properties in serialization/openapi spec generation, so, even though both fields are private in their class, there should be no issue with serialization. In looking at the code again, I think we are making more work to use the DTO classes vs. just using the regular classes. So, I recommend we keep things as I submitted in this PR.

Fine. You could add @Schema annotations to PersistenceStrategy and PersistenceCronStrategy to satisfy copilot.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/dto/PersistenceCronStrategyDTO.java:1

• The deletion of PersistenceCronStrategyDTO.java will cause a compilation failure. This DTO class is still actively used in two other files that were not updated as part of this PR:

1. PersistenceServiceConfigurationDTO.java (line 43): public Collection<PersistenceCronStrategyDTO> cronStrategies = List.of();
2. PersistenceServiceConfigurationDTOMapper.java (line 33): import org.openhab.core.persistence.dto.PersistenceCronStrategyDTO;, plus usage at lines 66, 82-83, and 177-181, where it's used to map cron strategy data.

Unlike PersistenceStrategyDTO (which was only used in an OpenAPI annotation in PersistenceResource.java and was cleanly removed after updating that annotation), PersistenceCronStrategyDTO serves a functional role in the persistence configuration DTO mapping infrastructure. Its name and cronExpression public fields are directly accessed by PersistenceServiceConfigurationDTOMapper. The PersistenceCronStrategy domain class cannot directly replace PersistenceCronStrategyDTO in PersistenceServiceConfigurationDTO because those domain classes use private fields with getter methods rather than public settable fields.

Either the deletion of PersistenceCronStrategyDTO.java must be reverted, or PersistenceServiceConfigurationDTO.java and PersistenceServiceConfigurationDTOMapper.java must be updated to no longer reference it.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

There is an extra blank line between the closing Javadoc comment (*/) and the @Schema annotation. The parent class PersistenceStrategy.java places the annotation immediately after the Javadoc without an extra blank line, so this inconsistency should be fixed by removing the extraneous blank line.

Suggested change

[Copilot]

The @QueryParam annotation for the item name filter on the GET /persistence/items endpoint was changed from "itemname" to "itemName". This is a breaking API change: any existing REST API clients using ?itemname=... to filter items will receive an unfiltered list instead (since the old itemname parameter will be ignored), and would need to switch to ?itemName=.... The description does not mention this change, and there is no indication in the PR context that this breaking change is intentional or coordinated with API consumers.

Suggested change

	@Parameter(description = "An Item name, if provided response will only contain information for this Item") @QueryParam("itemName") @Nullable String itemName) {
	return getServiceItemListDTO(serviceId, itemName);
	@Parameter(description = "An Item name, if provided response will only contain information for this Item") @QueryParam("itemName") @Nullable String itemName,
	@Parameter(description = "An Item name (legacy parameter), if provided response will only contain information for this Item") @QueryParam("itemname") @Nullable String legacyItemName) {
	String effectiveItemName = itemName != null ? itemName : legacyItemName;
	return getServiceItemListDTO(serviceId, effectiveItemName);

[jsjames]

• fixed the PersistenceStrategyDTO
• reverted to using PersistenceStrategyDTO and PersistenceCronStrategyDTO in the Schema annotations
• The DTOs still are not utilized in actually transferring the data - but that is a much more involved change

[florian-h05]

LGTM!

I agree with Mark that ideally the DTOs should be used to transfer the data. Having interfaces is always a bit risky, we’ve already had issues with this for the persistence item info endpoint in combination with InMemory persistence, which was using an anonymous implementation of the interface, causing serialisation to return null.

[holgerfriedrich]

@florian-h05 @kaikreuzer do we need to notify other maintainer groups?
I am not sure which frontends use which parts of the REST API.

[kaikreuzer]

Might be a good idea, especially @openhab/ios-maintainers and @openhab/android-maintainers.

[florian-h05]

do we need to notify other maintainer groups?

Annotation changes should be pretty risk-less, the actual endpoints should continue to work as before. Especially when the annotation changes correct wrong annotations, but it’s always better to check I guess.

[timbms]

@holgerfriedrich can you please post as a convenience for me the resulting openapi json spec. I could then compare with the spec I had to adjust manually to make it work with the swift openapi generator for the iOS app.

[jsjames]

@holgerfriedrich can you please post as a convenience for me the resulting openapi json spec. I could then compare with the spec I had to adjust manually to make it work with the swift openapi generator for the iOS app.

I generated a version as part of this PR: openhab/openhab-webui#3992