Copilot commented on code in PR #12947:
URL: https://github.com/apache/trafficserver/pull/12947#discussion_r2898212161
##########
doc/admin-guide/monitoring/error-messages.en.rst:
##########
@@ -108,6 +108,45 @@ it would be used instead of ``cache#read_error`` if there
is no ``apache_cache#r
The text for an error message is processed as if it were a
:ref:`admin-logging-fields` which
enables customization by values present in the transaction for which the error
occurred.
+.. _body-factory-info:
+
+Template Set Metadata
+---------------------
+
+Each template set directory may contain a :file:`.body_factory_info` file that
provides metadata
+about the error pages in that directory. This file controls the
``Content-Type``,
+``Content-Language``, and character set of the HTTP response headers sent with
error pages.
+
+The following directives are supported:
+
+``Content-Language``
+ The natural language of the error pages. This value is sent in the
``Content-Language`` HTTP
+ response header. Default: ``en``.
+
+``Content-Charset``
+ The character encoding of the error pages. This value is appended to the
``Content-Type`` header
+ as a ``charset`` parameter. Default: ``utf-8``.
+
+``Content-Type``
+ The MIME type for the error response. This controls the media type portion
of the ``Content-Type``
+ HTTP response header. Default: ``text/html``.
+
+For example, to serve plain text error pages in English::
+
+ Content-Language: en
+ Content-Charset: utf-8
+ Content-Type: text/plain
+
+This would produce the response header ``Content-Type: text/plain;
charset=utf-8``.
+
+To describe Korean error pages encoded in the ``iso-2022-kr`` character set::
+
+ Content-Language: kr
+ Content-Charset: iso-2022-kr
+
+If the file is empty, contains only comments, or is absent, the defaults are
used: English
+``text/html`` in the ``utf-8`` character set.
Review Comment:
The docs say each template set directory "may contain" a .body_factory_info
file and that if the file is absent defaults are used. In code,
load_body_set_from_directory() requires .body_factory_info to exist (missing
file causes the entire template set to be skipped), so an absent file does not
behave the same as an empty/comment-only file. Please adjust this section to
reflect that .body_factory_info is required to load a template set (but may be
empty/comments to use defaults).
```suggestion
The ``.body_factory_info`` file must exist for a template set to be loaded.
If the file is empty or
contains only comments, the defaults are used: English ``text/html`` in the
``utf-8`` character set.
```
##########
src/proxy/http/HttpBodyFactory.cc:
##########
@@ -181,7 +182,8 @@ HttpBodyFactory::fabricate_with_old_api(const char *type,
HttpTransact::State *c
if (buffer) { // got an instantiated template
if (!plain_flag) {
snprintf(content_language_out_buf, content_language_buf_size, "%s",
lang_ptr);
- snprintf(content_type_out_buf, content_type_buf_size, "text/html;
charset=%s", charset_ptr);
+ const char *mime_type = type_ptr ? type_ptr : "text/html";
Review Comment:
Same naming concern as above: `mime_type` is derived from `type_ptr`, but
the pointer name doesn’t communicate that it’s the response Content-Type value.
Renaming the pointer variable would improve readability and reduce the chance
of mixing it up with the error-page `type`.
```suggestion
const char *content_type_ptr = type_ptr;
const char *mime_type = content_type_ptr ? content_type_ptr :
"text/html";
```
##########
src/proxy/http/HttpBodyFactory.cc:
##########
@@ -77,6 +77,7 @@ HttpBodyFactory::fabricate_with_old_api(const char *type,
HttpTransact::State *c
char *buffer = nullptr;
const char *lang_ptr = nullptr;
const char *charset_ptr = nullptr;
+ const char *type_ptr = nullptr;
Review Comment:
The new local variable name `type_ptr` is easy to confuse with the existing
`type` parameter (error template name). Renaming it to something like
`content_type_ptr` / `mime_type_ptr` would make it clearer that this comes from
the .body_factory_info Content-Type directive.
```suggestion
char *buffer = nullptr;
const char *lang_ptr = nullptr;
const char *charset_ptr = nullptr;
const char *content_type_ptr = nullptr;
```
--
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]
