DhroovSankla commented on PR #6017:
URL: https://github.com/apache/fineract/pull/6017#issuecomment-4810091297
> > > 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.
Thank you so much for this detailed architectural breakdown! That makes
perfect sense. I completely see how explicit wrapping duplicates what JAX-RS
natively handles under the hood via the existing annotations.
Your clarification on the true intent of the `TODO` is incredibly helpful.
Shifting the architecture to return a proper Response DTO/domain object
annotated with Jakarta XML annotations—and letting the framework automatically
serialize it—is a much more impactful improvement to the codebase than wrapping
the raw string.
Since this current implementation doesn't provide additional structural
value, I will go ahead and close this PR. I would love to open a fresh,
properly scoped ticket/PR to refactor `MixReportXBRLData` into a clean,
framework-serialized XML DTO as you suggested.
Thank you again for the mentorship and guidance here!
--
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]
