Copilot commented on code in PR #12947: URL: https://github.com/apache/trafficserver/pull/12947#discussion_r2983457891
########## tests/gold_tests/body_factory/body_factory_content_type.test.py: ########## @@ -0,0 +1,105 @@ +''' +Tests that the Content-Type directive in .body_factory_info is honored +for body factory error responses. +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os + +Test.Summary = 'Verify Content-Type directive in .body_factory_info controls error response MIME type' +Test.ContinueOnFail = True + + +class BodyFactoryContentTypeTest: + """ + Test that the Content-Type directive in .body_factory_info is used for + body factory error responses instead of the hardcoded text/html default. + + Two scenarios: + 1. Default: no Content-Type directive -> text/html; charset=utf-8 Review Comment: The scenario description in this docstring says the default case returns `text/html; charset=utf-8`, but the test below only asserts `text/html` (no charset) for the default path. Please update the docstring to match the behavior being tested. ```suggestion 1. Default: no Content-Type directive -> text/html ``` ########## src/proxy/http/HttpTransact.cc: ########## @@ -8359,6 +8359,8 @@ HttpTransact::build_error_response(State *s, HTTPStatus status_code, const char if (len > 0) { s->hdr_info.client_response.value_set(static_cast<std::string_view>(MIME_FIELD_CONTENT_TYPE), body_type); s->hdr_info.client_response.value_set(static_cast<std::string_view>(MIME_FIELD_CONTENT_LANGUAGE), body_language); + ats_free(s->internal_msg_buffer_type); + s->internal_msg_buffer_type = ats_strdup(body_type); Review Comment: The PR description mentions fixing `HttpSM::setup_internal_transfer` to only apply the `text/html` default when `Content-Type` is not already present, but `setup_internal_transfer()` still unconditionally sets `Content-Type: text/html` when `internal_msg_buffer_type` is null. If the intent is to preserve a `Content-Type` set earlier in the response pipeline (e.g., by a plugin), this behavior still overwrites it unless the plugin also populates `internal_msg_buffer_type`. ########## 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 must contain a ``.body_factory_info`` file for the template set to be +loaded. 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``. + Review Comment: The `Content-Charset` directive is documented as always being appended to the `Content-Type` header as a `charset` parameter, but the current implementation only appends `; charset=...` when a `Content-Type` directive is present and does not already include `charset=`. Consider updating this section to reflect the actual behavior (including what happens when `Content-Type` is omitted or already has a charset parameter). -- 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]
