Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix polymorphism in Translator #1346

Conversation

marc-vanderwal
Copy link
Contributor

@marc-vanderwal marc-vanderwal commented May 15, 2024

Purpose

This PR fixes some regressions, presumably introduced by #1319, that could not be caught in Zonemaster::Engine’s unit tests.

Context

Problems

In Zonemaster::Backend, there is a Zonemaster::Backend::Translator class which inherits from Zonemaster::Engine::Translator. This is done in order to override the translate_tag method with a custom version.

When “un-Moosing” Zonemaster::Engine, however, changes were introduced that were harmless for Zonemaster::Engine, but not for Zonemaster::Backend. This caused two issues to emerge:

  • firstly, commands such as zmb get_test_results, that rely on translating messages pulled from the database, stopped working;
  • secondly, messages generated by the backend itself were no longer translated.

Cause

The root cause of the first problem is that when calling Zonemaster::Backend::Translator->new(), the newly-created instance was always blessed as Zonemaster::Engine::Translator (the superclass), not as the inheriting class. Thus, translate_tag() method calls always called the method in the superclass instead of the inheriting class.

As for the second problem, this is due to Backend’s Translator overriding another method, _build_all_tag_descriptions, from Engine’s Translator, and the initialization code in Zonemaster::Engine::Translator failing to call the overridden _build_all_tag_description when invoked as the Backend’s Translator class.

Changes

  • In the constructor, instead of blessing with __PACKAGE__ (a macro that always expands to Zonemaster::Engine::Translator), bless with $class instead.
  • Make the _build_all_tag_descriptions subroutine a proper class method and have initialize hand over to the method of the correct class.

How to test this PR

  1. Make sure Ensure Backend’s messages stay translated zonemaster-backend#1166 is merged first.

  2. Run a test with zmb and grab the test ID (or have a test ID of a previous test handy and skip the following command):

TEST_ID=`zmb start_domain_test --domain localhost | jq -r .result`
  1. Then, run the following after the test is done:
zmb get_test_results --lang fr --test-id $TEST_ID | jq -e .result.params.domain

This command should print "localhost" (or whatever the domain under test was) and set an exit status of 0. Any other result, or null, is abnormal.

  1. Using zmtest, run a test on a domain name that causes Zonemaster to time out, such as pool.ntp.org. The error message stating that the test took too long to complete should be translated (i.e., the raw untranslated message tag must not appear).

In Zonemaster::Backend, the codebase defines a
Zonemaster::Backend::Translator class which inherits from
Zonemaster::Engine::Translator, because it needs to override the
translate_tag() method with a custom version.

However, one change introduced in zonemaster-engine#1319 caused this
inheritance to no longer work properly.

The main symptom was that commands such as `zmb get_test_results`, that
rely on translating messages pulled from the database, stopped working.

The root cause of the problem is that when calling
Zonemaster::Backend::Translator->new(), the newly-created instance was
always blessed as Zonemaster::Engine::Translator (the superclass), not
as the inheriting class. Thus, the translate_tag() method was always
that of the superclass instead of the inheriting class. Hence the crash.

Fixing this is easy, fortunately.
@marc-vanderwal marc-vanderwal added T-Bug Type: Bug in software or error in test case description P-High Priority: Issue to be solved before other labels May 15, 2024
@marc-vanderwal marc-vanderwal added this to the v2024.1 milestone May 15, 2024
@matsduf
Copy link
Contributor

matsduf commented May 15, 2024

In the "how to test" it should be jq -r .result not jq -r result, at least with the jq I have.

@marc-vanderwal
Copy link
Contributor Author

In the "how to test" it should be jq -r .result not jq -r result, at least with the jq I have.

That’s because it was a typo. I’ve fixed the procedure.

The _build_all_tag_descriptions should be called using
dynamic (polymorphic) dispatch.

Backend has a Translator that inherits from the Engine’s Translator, and
overrides _build_all_tag_descriptions to call this class’s method, then
does some processing of its own.

Without this change, some Backend-specific error messages will remain
untranslated.
@marc-vanderwal marc-vanderwal added the V-Patch Versioning: The change gives an update of patch in version. label May 16, 2024
@marc-vanderwal marc-vanderwal merged commit 8976c69 into zonemaster:develop Jun 3, 2024
3 checks passed
@marc-vanderwal marc-vanderwal deleted the bugfix/broken-translator-polymorphism branch June 12, 2024 09:54
@mattias-p mattias-p self-assigned this Jun 25, 2024
@mattias-p
Copy link
Member

Release testing for v2024.1

I've tested this according to the testing instruction above with successful results.

@mattias-p mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-High Priority: Issue to be solved before other S-ReleaseTested Status: The PR has been successfully tested in release testing T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants