Improvement: Handling status codes returned by Repository Verticle

[marcinczeczko]

Repository Verticle while fetching content from Http Repository can get 200 response with content which is fine and clear how to handle. However there are more scenarios we need to consider:

Http repository might return

• 404 -> we should back to client as 404
• 500 -> should we back 500 to client ( can be missleading as it would suggest something wrong with Knotx) ? or 404 (might be more clean, template not found on remote repo, becuase it responsed with 500)
• 302 - returned by AEM publisher if CUG are implemented. It return 302 redirect to login page - How should we deal with it ?
• 301 - permament redirect - how should we deal with it ?
• Other codes ??

What about if we added feature allowing to define on configuration level how to map Http Repository responses into client reposnse status codes ? Just high level idea to start a discussion..

"remote.repo.status.code.mapping": {
    "200": {   #repo response 200 mapped to
         "code": 200,  # client response 200
         "headers": true # with headers from repository
    },
    "404": {
         "code": 200,
         "headers": false
    },
    "302":  {
         "code": 400,
         "headers": false
    }
}

[tomaszmichalak]

500 -> should we back 500 to client ( can be missleading as it would suggest something wrong with Knotx) ? or 404 (might be more clean, template not found on remote repo, becuase it responsed with 500)

I think that 400 is better in that case. 500 errors should be reserved for Knotx and services (if service configured in that way).

302 - returned by AEM publisher if CUG are implemented. It return 302 redirect to login page - How should we deal with it ?

Knot.x can be connected to different repository sources. In production architecture Apache Front should handle redirects to login pages. Knot.x should not be aware of that.

301 - permament redirect - how should we deal with it ?

Simply return 301 to client.

Other codes ??

I think that Knot.x should return all 3XX, 4XX error code. All 5XX code should be transformed to 400.

[wiiitek]

The 400 error code means client error. This may be misleading as the request itself could be correct. I think we should return the response codes transparently as they have their meaning.

Let's have knot.x simple, doing one thing and doing it good.

[malaskowski]

I think, that Knot.x should not worry about status codes returned by http implementation of repository. There should be one clear message: "Should markup be processed by template engine or not". There is special case with redirects but this is not Knot.x responsibility to interpret or act on specific status codes. This is Apache deal. Please remember that http is not the only (but probably the most used) implementation of repository. E.g. there is fileSystem implementation, which decides what to do with 'FileNotFound' or 'IO' exceptions.

To sum up: Knot.x should return 3XX, 4XX error codes with headers, all 5XX code should be transformed to 404. But this decision should be made by repository implementation, message from repository to server should be: "Process by templating engine" or "Error, finish".

[marcinczeczko]

I see you point and agree that Knotx shouldn't implement extra logic in this case. In my opinition it should be like this:

Http Repository

• Repo returned 200 with body-> return 200 + body+ headers
• Repo returned 200 with empty body -> return 404 + headers
• Repo returned 2XX (except 200), 3XX, 4XX -> return original code + body(if exists) + headers
• Repo returned 5XX -> return 404 + body (if exists) + headers

Local Repository

• Repo returned not empty content -> return 200 + body
• Repo returned empty content -> return 404
• Repo unable to get content (ioexception or others) -> return 404

[tomaszmichalak]

Repo returned 200 with empty body -> return 404 + headers

This is a bit tricky and add additional rule. Do we want to validate content if repository returns 200?

What do you think about: Http Repository Repo returned 2XX, 3XX, 4XX -> return original code + body(if exists) + headers Repo returned 5XX -> return 404 + body (if exists) + headers

[marcinczeczko]

Basically this is the same, except that we will return empty content if repo will return it also as empty - the question is if we ok with that ?

[marcinczeczko]

As we discussed offline the approach should be very simple. Knot.x shouldn't be reponsible for doing some mappings over status codes returned by repository. The approach should be as follows:

Remote repo

• If statusCode=200 and has body - do the processing
• If statusCode=200 and no body - return as it is (empty body) keeping the headers and statusCode from repository
• If statusCode <> 200 - return as it is keeping the headers and statusCode from the repository

Local repo

• If local repo successfully retrieved file (has body) - do the processing
• If local repo successfully retrieved file (empty body) - return as it is (200)
• If local repo failed due FileNotFoundException - return 404
• If local repo failed due other error - return 500

How to return repositoryResponse

• For 200 with not empty body - use as it's now RepositoryReponse.success()
• For 200 with empty body - we need new factory RepositoryReponse.empty() - to return response that is kind of success but not error - this kind of response should be handled by server verticle and return as it's without processing by engine. RepositoryReponse
• For other codes - use RepositoryReponse.error() as it's now
• RepositoryReponse should have method

boolean shouldProcess()

That returns

• true only for success response and not empty body
• false otherwise

shouldProcess() should be used in server verticle to decide if request should be processed by engine (shouldProcess=true) or returned as it is (shouldProcess=false)

• RepositoryResponse should be an abstract class
• Inherited three classes: EmptyRepositoryReponse (shouldProcess=false), ErrorRepositoryReposse(shouldProcess=false), SuccessRepositoryResponse(shouldProcess=true)
• PseudoCode for RepositoryReponse

abstract class RepositoryReponse {
   protected int code;
   protected String data;
   protected MultiMap headers;

   abstract boolean shouldProcess();

   public int getCode() {...}
   public int getData() {...}
   public MulitMap getHeaders() {...}

  pubic static RepositoryReponse error(code, data, headers) {
     return new ErrorRepositoryReponse(code, data, headers);
  }
  pubic static RepositoryReponse empty(headers) {
     return new EmptyRepositoryReponse(headers); //EmptyRepositoryReponse will set code=200
  }
  pubic static RepositoryReponse success(content, headers) {
     return new SuccessRepositoryReponse(content, headers); //will set code=200
  }
 }

[malaskowski]

Fixed by #359