[multicast_dns] Allow decode of binary encoded TXT

[ajhemphill91]

Follow up on https://github.com/flutter/flutter/issues/141218. Added a unit test with a binary-encoded payload.

Pre-launch Checklist

• [ ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [ ] I read the Tree Hygiene page, which explains my responsibilities.
• [ ] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [ ] I signed the CLA.
• [ ] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [ ] I linked to at least one issue that this PR fixes in the description above.
• [ ] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [ ] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• [ ] I updated/added relevant documentation (doc comments with ///).
• [ ] I added new tests to check the change I am making, or this PR is test-exempt.
• [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ajhemphill91]

Side note, the decoding algorithm for TXT data is still not exactly in spec (see RFC6763 6.3-6.5).

In particular:

DNS-SD TXT record strings beginning with an '=' character (i.e., the key is missing) MUST be silently ignored.

The characters of a key MUST be printable US-ASCII values (0x20-0x7E) [RFC20], excluding '=' (0x3D).

These don't matter to me too much at the present time (and I have made no changes in this PR regarding them). It does look like the openthread border router's implementation has these restrictions on the '=' character within the key.

[stuartmorgan-g]

From triage: ping @dnfield on this review

[jmagman]

Hi @ajhemphill91, have you been able to circle back to the requested change for the named constructor? Appreciate your contribution!

[ajhemphill91]

Sorry for the delay, I have been on another project recently. If you do have a preference to keep the original constructor (and thus the original final String text property) for API stability, your suggested changes sound good. Internally, this constructor is only referenced once in packet.dart and then in the unit tests which are updated by this commit.

The text property as a String is not valid since it represents a list of encoded strings which are treated as binary information. Basically it mutates the record by adding the \n characters.

Actually now that I am looking at it, encodeResponseRecord ought to fail to produce correct results too since the record is mutated by the text property. However it looks as though this property is never referenced anywhere.

[stuartmorgan-g]

From triage: @ajhemphill91 Are you still planning on updating this PR?

[ajhemphill91]

I can apply the nit, but changing back to a named constructor that joins the records with a newline nullifies this PR in the first place. What would you like me to do?

[jmagman]

Sorry, didn't realize this PR was waiting for my response.

but changing back to a named constructor that joins the records with a newline nullifies this PR in the first place

Hm, yeah I can't think of a way to do this without something unpleasant like a text.split('\n')

  TxtResourceRecord(
    String name,
    int validUntil, {
    required this.text,
  })  : attrs = text.split('\n'),
        super(ResourceRecordType.text, name, validUntil);

  TxtResourceRecord.attributes(
    String name,
    int validUntil, {
    required this.attrs,
  })  : text = attrs.join('\n'),
        super(ResourceRecordType.text, name, validUntil);

Adding @cbracken for his opinion here. Ideally this could be done without a breaking change.

[stuartmorgan-g]

I looked at the usage graph for multicast_dns, and very few published packages rely on it, so a breaking change won't be particularly disruptive (since the chances of a version incompatibility in multiple dependencies is very low). Given that, and that it doesn't seem like we can correctly populate the attributes with the existing constructor, I think doing a breaking change here is fine. Especially since updating to the new major version is likely to require test-only changes at most.

[stuartmorgan-g]

@ajhemphill91 I've re-added the checklist from the template to the PR description; in the future please don't remove it. This PR is missing several required steps (notably for this discussion, a version change).

[ajhemphill91]

Per the versioning guide should I then update the pubspec.yaml version to 0.4.0? I can take care of that and the CHANGELOG.md if everyone is okay with the breaking change.

[stuartmorgan-g]

@jmagman Are you okay with the breaking change?

[jmagman]

@jmagman Are you okay with the breaking change?

LGTM, I can't remember if this will impact the tool but it would be easy to fix up

[cbracken]

@ajhemphill91 Looks like Dart formatting and CHANGELOG format checks are failing. See the log here for details: https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20repo_checks/4671/overview

Once those are done, I leave it up to you as to whether you want to do the multi-step dance to go through the deprecation, then breaking change removal, or just go straight to the breaking change and land this as-is.

[stuartmorgan-g]

@ajhemphill91 Is this still something you are planning on updating?