search

Knotx / knotx-fragments

Fragments map-reduce processing using Graph flows, supplier and assembler.
https://knotx.io
Apache License 2.0
3 stars 5 forks source link

HtmlFragmentsSupplier to be able to consume 302 response with no html body #161

Closed ub1k24 closed 4 years ago

ub1k24 commented 4 years ago

Is your feature request related to a problem? Please describe. HtmlFragmentsSupplier processes 302 same as 200 responses which makes it fail on the lack of html body in the response

Describe the solution you'd like HtmlFragmentsSupplier should have an ability to pass with no error 302 responses directly through to further handlers

Describe alternatives you've considered HtmlFragmentsSupplier could be configurable to allowEmpty html body - with a config flag

Additional context This causes AEM integration fail on authoring, whenever user accesses a page through knotx when not logged in

tomaszmichalak commented 4 years ago

In scope:

malaskowski commented 4 years ago

I reviewed the logic from the HtmlFragmentsSupplier one more time. And I must admit I find it quite bad decision to throw an exception when supplier is not able to produce any fragments. I guess that's too much responsibility, which should be handled somewhere else (e.g. at the Repository Connector level). I'd suggest to remove the check for empty body and just produce an empty list of fragments, when the supplier is not able to provide anything meaningful from the request context (client response). @tomaszmichalak , @ub1k24 WDYT?

ub1k24 commented 4 years ago

From my perspective it's perfectly fine to do as you say. This was my initial thought that HtmlFragmentsSupplier should not fail on empty body.

malaskowski commented 4 years ago

There is one more logic to consider. Currently FragmentsAssemblerHandler sets status code 200 when merging fragments. It wasn't a problem previously, when there was no body and exception was thrown in the HtmlFragmentsSupplier. Currently, when empty fragments list is processed by FragmentsAssemblerHandler it still overrides the status code (SC) to 200.

I can think of following scenarios to update that logic, so that 302 is returned in the server response:

  1. Update FragmentsAssemblerHandler SC logic

    • when request context contains SC (in client response) - use that SC
    • otherwise (no SC in cilent response) set 200 only when body is not empty (case for API Gateway & Web API)

  2. Set 200 in client response at the beginning of processing and remove logic for SC from assembler

  3. Update ResponseWriterHandler (exactly ServerResponse) logic to set SC according to:

    • if there is SC in clientResponse - use that SC
    • otherwise if there is body and no SC -> 200
    • otherwise (no body, no SC) -> 500

What is worth remembering, any Handler can modify the clientResponse SC (currently there are 3 such handlers: both repo connector, and assembler). I hesitate between options (1) (change scope limited to fragments repository) and (3) (looks like a proper place to have SC logic in one place).

Edit

It looks that (2) is already happening: https://github.com/Knotx/knotx-server-http/blob/2.2.0/api/src/main/java/io/knotx/server/api/context/RequestContext.java#L42