Separate functions to trim whitespace and normalize domain name by mattias-p · Pull Request #1316 · zonemaster/zonemaster-engine

mattias-p · 2024-01-08T17:56:25Z

Purpose

This PR separates the normalization of what's logically part of domain names from the white space stripping that is also specified in RequirementsAndNormalizationOfDomainNames.md to make it possible to apply them individually.

Context

This PR fixes item 3 in zonemaster/zonemaster-cli#364.

zonemaster/zonemaster-backend#1143 should be merged together with this one.

This is technically a major change because normalize_name() is part of the public API. However, even though we've included an implementation in prior releases, we've never actually called it (except from unit tests).

Changes

This PR splits out trim_space() from normalize_name().
As a drive-by change it also improves the quality of the associated unit test.

How to test this PR

Zonemaster CLI should present the following behaviors:

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 'example .com'
Domain name has an ASCII label ("example ") with a character not permitted.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 'example '
Domain name has an ASCII label ("example ") with a character not permitted.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 ' '
Domain name has an ASCII label (" ") with a character not permitted.

$ zonemaster-cli --raw --level info --no-ipv6 --test basic01 zonemaster.fr --ns 'd.nic.fr. /194.0.9.1'
Invalid name in --ns argument:
    Domain name has an ASCII label (" ") with a character not permitted.

$ zonemaster-cli --raw --level info --no-ipv6 --test basic01 zonemaster.fr --ns 'd.nic.fr./ 194.0.9.1'
Invalid IP address in --ns argument:
    Invalid chars in IP  194.0.9.1

mattias-p · 2024-03-04T11:06:05Z

@matsduf has expressed that he wants to update RequirementsAndNormalizationOfDomainNames.md before this is merged.

matsduf

If trimming of leading and trailing white space is removed from Engine, then the function must be in GUI or in Backend. Do we have the function in GUI?

Another option would be that Engine offers two alternatives, trimming of leading and trailing white space enabled and trimming disabled, respectively. That would make it easy for a new client to choose.

Also, we have zmb in Backend. Should that behave like GUI and trim? Or should that be like CLI and not trim? If zmb should trim (to make it more GUI like) then trimming should be done in Backend.

mattias-p · 2024-07-22T17:37:41Z

@matsduf Regarding your comment above, as you remember, we discussed this topic at a work group meeting a while ago. I believe it was after your comment, but it seems we forgot to note our conclusions here. Instead of leaving (seemingly) unanswered questions hanging, it seems appropriate to summarize some clarifications along the lines of our discussion as I remember it.

The test case specifications should not specify the trimming of surrounding space as part of the definition of any of its data types (e.g. domain names). It is questionable if the test case specifications need to specify space trimming at all. But if they do, they should specify a common trimming algorithm that can be applied (or avoided) at the discretion of user interface implementations to conform whatever expectations apply to that particular kind of user interface.
GUI should trim spaces from values in any text field (e.g. domain name fields) before doing anything with them (e.g. before sending them to Backend). This is what you'd expect from a web application and we don't have any reasons to deviate.
RPCAPI should not trim space from any of its arguments. It exposes an application programming interface and clients are expected to say what they mean and not be sloppy in their requests.
Neither zmb nor zmtest should trim space from any of its arguments. You wouldn't expect a command line tool to do that.
Additionally, zmb should definitely not trim space from any of its arguments because we need it to be able to test how Backend responds to requests both with and without surrounding space in all request arguments.

@matsduf, @marc-vanderwal, @tgreenx, @MichaelTimbert do you guys agree with the above?

matsduf · 2024-07-23T12:31:50Z

@mattias-p, please see zonemaster/zonemaster#1260.

matsduf · 2024-07-23T12:37:18Z

@mattias-p, you do not trimming to be part of the normalization specification (you write "test case specification" but I think you mean the normalization document), but you want the normalization to happen in GUI. Where do you want that to be specified or do you prefer it to be unspecified.

I am not sure that I agree with your bullets, but I will not totally object. A requirement is that operations happening are documented and specified.

Also, fix resulting compilation errors and make iteration order deterministic.

* Better diagnostics. * More complete test for bad domain names.

marc-vanderwal

Code looks fine; just one nit.

marc-vanderwal · 2025-05-28T09:47:52Z

-use Test::More;
-use Test::Exception;
-
+#!perl


.t files don’t need shebangs, do they?

It does not hurt, does it? It can help some editors to recognize it as a Perl script.

Strictly speaking, I don't think it's required. I picked it up as a habit at some point, I don't remember exactly why. It seems at this point it's mostly a historical artifact. I noticed that it's present in examples in Intermediate Perl from 2012.

matsduf · 2025-05-28T14:46:22Z

Before this PR is merged, zonemaster/zonemaster#1260 should be approved and merged.

matsduf

Fine given that zonemaster/zonemaster#1260 is approved and merged.

matsduf · 2025-06-02T08:01:10Z

zonemaster/zonemaster#1260 has been merged. How is this PR related to zonemaster/zonemaster-backend#1143

mattias-p · 2025-06-02T09:05:11Z

This is a breaking change, and zonemaster/zonemaster-backend#1143 depends on this change.

tgreenx · 2025-06-12T14:00:15Z

v2025.1 Release testing

Works as advertised in the "How to test" section.

mattias-p added the V-Major Versioning: The change gives an update of major in version. label Jan 8, 2024

mattias-p changed the title ~~Normalization~~ Separate functions to trim whitespace and normalize domain name Jan 8, 2024

mattias-p added this to the v2023.2 milestone Jan 8, 2024

mattias-p mentioned this pull request Jan 10, 2024

Adapt to changed Engine API zonemaster/zonemaster-backend#1143

Merged

matsduf modified the milestones: v2023.2, v2024.1 Jan 16, 2024

mattias-p mentioned this pull request Apr 30, 2024

Separate handling of surrounding space from normalization in specification zonemaster/zonemaster#1259

Closed

matsduf mentioned this pull request Apr 30, 2024

Makes removal of leading and trailing space optional when domain name is normalized zonemaster/zonemaster#1260

Merged

matsduf requested changes May 2, 2024

View reviewed changes

matsduf modified the milestones: v2024.1, v2024.2 Jun 12, 2024

mattias-p modified the milestones: v2024.2, v2025.1 Nov 13, 2024

mattias-p added 3 commits May 27, 2025 08:25

Add strictures

4618c86

Also, fix resulting compilation errors and make iteration order deterministic.

Improved unit test

1d35a4d

* Better diagnostics. * More complete test for bad domain names.

Split out trim_space() from normalize_name()

bc0967e

mattias-p force-pushed the normalization branch from 0fb2404 to bc0967e Compare May 27, 2025 06:26

marc-vanderwal reviewed May 28, 2025

View reviewed changes

matsduf requested review from MichaelTimbert, tgreenx and tolvmannen May 28, 2025 14:54

matsduf approved these changes May 28, 2025

View reviewed changes

tgreenx approved these changes May 28, 2025

View reviewed changes

matsduf merged commit 84ab5aa into zonemaster:develop Jun 2, 2025
3 checks passed

mattias-p deleted the normalization branch June 2, 2025 13:32

tgreenx added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 12, 2025

tgreenx mentioned this pull request Jul 1, 2025

Improved handling of domain names in command line arguments zonemaster/zonemaster-cli#364

Closed

tgreenx linked an issue Jul 1, 2025 that may be closed by this pull request

Improved handling of domain names in command line arguments zonemaster/zonemaster-cli#364

Closed

Uh oh!

Conversation

mattias-p commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Context

Changes

How to test this PR

Uh oh!

mattias-p commented Mar 4, 2024

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

mattias-p commented Jul 22, 2024

Uh oh!

matsduf commented Jul 23, 2024

Uh oh!

matsduf commented Jul 23, 2024

Uh oh!

marc-vanderwal left a comment

Choose a reason for hiding this comment

Uh oh!

marc-vanderwal May 28, 2025

Choose a reason for hiding this comment

Uh oh!

matsduf May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattias-p Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

matsduf commented May 28, 2025

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

matsduf commented Jun 2, 2025

Uh oh!

mattias-p commented Jun 2, 2025

Uh oh!

Uh oh!

tgreenx commented Jun 12, 2025

v2025.1 Release testing

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattias-p commented Jan 8, 2024 •

edited

Loading

matsduf May 28, 2025 •

edited

Loading