adamsaghy commented on PR #6017:
URL: https://github.com/apache/fineract/pull/6017#issuecomment-4809965330
> > I dont really see any additional value of this PR.
> > @DhroovSankla Can you please advise what were you trying to improve here?
>
> Hi @adamsaghy,
>
> Thank you for reviewing this PR!
>
> The primary intention here was to address the legacy technical debt
comment left inline within this module: `// TODO: make this type safe?`.
>
> Architecturally, wrapping the XML string payload inside a JAX-RS
`Response` object decouples the business layer data from the transport
configuration. By returning a formal `Response` entity instead of a raw
`String`, it allows explicit framework control over response metadata, headers,
and content negotiation via `.type(MediaType.APPLICATION_XML_TYPE)`. This
future-proofs the resource for adding alternative status codes or custom
headers seamlessly down the line without changing the core method signature
logic.
>
> Please let me know if you would prefer to retain the legacy raw String
approach or if this architectural alignment fits the current standards of the
project!
There is a reason why you dont see this `Response` object wrapping anywhere
in the codebase: because its useless...
Let's review what is happening:
`return
Response.ok().entity(xmlPayload).type(MediaType.APPLICATION_XML_TYPE).build();`
- `Response.ok()` -> Explicitly map the result to HTTP 200: That is the
default behaviour anyway, so it is unnecessary...
- `.entity(xmlPayload)` -> Mark `xmlPayload` as the body of the HTTP
response: That is the default behaviour when returning any object anyway, so it
is unnecessary ..
- `.type(MediaType.APPLICATION_XML_TYPE)` -> Explicitly set the return
response type is XML type: `@Produces({ MediaType.APPLICATION_XML })`
annotation on the method already doing this, so it is kinda useless once again
and duplication.
While it is no incorrect, but bring no any value: this is exactly what
happens anyway behind the curtains...
I think the real question and the TODO was referring to the manual building
of the xml then the serialization of it (to String) instead of allowing Jax-rs
to do it for us: the proper way would be to annotation the `MixReportXBRLData`
or a response DTO object with XML annotations and allow the framework to do the
xml building and serialization automatically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
