Distinguish present but empty and not present metadata for item functions by JaynieBai · Pull Request #8411 · dotnet/msbuild

JaynieBai · 2023-02-07T14:13:05Z

Context

The referenced function GetMetadataEscaped of Item metadata function dosen't distinguish between "metadata not present" and "present but set the empty string". Both of them return the same empty string. So the following functions

AnyHaveMetdataValue
HasMetadata
WithMetadataValue

Can't tell between "metadata not present" and "present but set the empty string".

Changes Made

Add GetMetadataValueEscaped with one more parameter returnNullIfNotFound that is true that returns null when not present.
Add one new function WithOutMetadataValue

Testing

MetadataFuntionTestingWithEmtpyString

Notes

…m functions

Forgind

Looks good!

ladipro

Looks great! Leaving a few comments inline.

ladipro · 2023-02-13T14:25:32Z

+            if (value == null) { return false; }
+            else { return true; }


nit:

Suggested change

if (value == null) { return false; }

else { return true; }

return value != null;

ladipro · 2023-02-13T14:32:17Z

+                if (string.IsNullOrEmpty(name))
+                {
+                    ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name));
+                }


Isn't this a breaking change? It looks like the method was previously OK with taking an empty string, now it throws.

Yes, I think it's necessary. HasMetadata parameter and GetMetadata parameter should have the same limitation. If name is null, Dictionary _directMetadata?.Contains(name) will throw exception.

That doesn't seem to be true. _directMetadata?.Contains(name) does not throw on null.

If there is a path that doesn't handle null, I think it would be reasonable to add a proper argument check here. I would be opposed to checking for an empty string, though, unless there is a good reason. The breaking potential is non-trivial.

Also, changes in the behavior of the public API should always come with the corresponding test coverage.

That doesn't seem to be true. _directMetadata?.Contains(name) does not throw on null.

If there is no null check. this will throw the exception as bellow. I think this exception message is not easier to undertand than ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name)); And for empty string check, I think it's same with GetMetadata. but in order to avoid new potential issues, I will only check null currently.

Have added the tests for the new public API

Sorry, you're right, it throws on null. Thank you for making the changes in HasMetadata. The new GetMetadataValueEscaped still doesn't take empty string now. Do you think it would make sense to unify the behavior with HasMetadata and validate the argument so it throw only on null?

The new GetMetadataValueEscaped still doesn't take empty string now. Do you think it would make sense to unify the behavior with HasMetadata and validate the argument so it throw only on null?

@Forgind what’s your idea？

ladipro · 2023-02-14T08:29:34Z

-            }
-
-            return value ?? String.Empty;
+            return GetMetadataValueEscaped(name, false);


super-nit:

Suggested change

return GetMetadataValueEscaped(name, false);

return GetMetadataValueEscaped(name, returnNullIfNotFound: false);

ladipro · 2023-02-14T11:41:49Z

+                if (string.IsNullOrEmpty(name))
+                {
+                    ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name));
+                }


That doesn't seem to be true. _directMetadata?.Contains(name) does not throw on null.

If there is a path that doesn't handle null, I think it would be reasonable to add a proper argument check here. I would be opposed to checking for an empty string, though, unless there is a good reason. The breaking potential is non-trivial.

Also, changes in the behavior of the public API should always come with the corresponding test coverage.

ladipro · 2023-02-15T12:38:36Z

+        /// Test metadata item functions with empty string metadata and not present metadata
+        /// </summary>
+        [Fact]
+        public void MetadataFuntionTestingWithEmtpyString()


nit: Typo Funtion.

ladipro · 2023-02-15T12:46:08Z

+        /// <summary>
+        /// Returns the metadata with the specified key.
+        /// Returns null if returnNullIfNotFound is true otherwise returns empty string when metadata not present
+        /// </summary>
+        string GetMetadataValueEscaped(string name, bool returnNullIfNotFound);


nit: Can you please move this up so the two overloads of GetMetadataValueEscaped are next to each other?

ladipro · 2023-02-15T12:58:27Z

+                if (string.IsNullOrEmpty(name))
+                {
+                    ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name));
+                }


Sorry, you're right, it throws on null. Thank you for making the changes in HasMetadata. The new GetMetadataValueEscaped still doesn't take empty string now. Do you think it would make sense to unify the behavior with HasMetadata and validate the argument so it throw only on null?

ladipro · 2023-02-15T13:04:21Z

+        /// Returns the metadata with the specified key.
+        /// Returns null if returnNullIfNotFound is true otherwise returns empty string when metadata not present
+        /// </summary>
+        public string GetMetadataValueEscaped(string name, bool returnNullIfNotFound)


Is it intentional to expose the method publicly? The existing overload is implemented explicitly, which keeps it internal.

Suggested change

public string GetMetadataValueEscaped(string name, bool returnNullIfNotFound)

string IItem.GetMetadataValueEscaped(string name, bool returnNullIfNotFound)

I think it‘s same with HasMetadata，SetMetadata and GetMetada， so make is public

True, HasMetadata, GetMetadataValue, and SetMetadataValue are public.

But the existing GetMetadataValueEscaped(string name) is not, so unless there's a need to expose the new GetMetadataValueEscaped(string name, bool returnNullIfNotFound), it should stay internal.

OK. I will update that.

rainersigwald

I have no problem adding WithoutMetadataValue, but I still don't think there should be a meaningful distinction between "metadata is set to empty" and "metadata is not set". Does the former require the latter?

JaynieBai · 2023-02-17T08:36:14Z

I have no problem adding WithoutMetadataValue, but I still don't think there should be a meaningful distinction between "metadata is set to empty" and "metadata is not set". Does the former require the latter?

…i/issue8205

…nd) internal

rainersigwald · 2023-02-28T22:56:05Z

if there is no distinction, WithoutMetadataValue('A', '') will output [One|Two] but it should be [One|Two|Four]

I don't think I agree. I think "undefined" and "defined but set to empty string" should be equivalent. Compare how these two work with your changes:

    <Message Importance="high" Text="WithoutMetadataValueAEmpty: [@(_Item->WithoutMetadataValue('A', ''), '|')]"/>
    <Message Importance="high" Text="BatchCondition: [@(_Item, '|')]" Condition=" '%(_Item.A)' != '' "/>

$ ./.dotnet/dotnet msbuild foo.proj
MSBuild version 17.6.0-dev-23128-01+cec2cb290 for .NET

  WithoutMetadataValueAEmpty: [One|Two|Four]
  BatchCondition: [One]
  BatchCondition: [Two]

I think they should match.

JaynieBai · 2023-03-03T08:13:04Z

if there is no distinction, WithoutMetadataValue('A', '') will output [One|Two] but it should be [One|Two|Four]

I don't think I agree. I think "undefined" and "defined but set to empty string" should be equivalent. Compare how these two work with your changes:
    <Message Importance="high" Text="WithoutMetadataValueAEmpty: [@(_Item->WithoutMetadataValue('A', ''), '|')]"/>
    <Message Importance="high" Text="BatchCondition: [@(_Item, '|')]" Condition=" '%(_Item.A)' != '' "/>
$ ./.dotnet/dotnet msbuild foo.proj
MSBuild version 17.6.0-dev-23128-01+cec2cb290 for .NET

  WithoutMetadataValueAEmpty: [One|Two|Four]
  BatchCondition: [One]
  BatchCondition: [Two]
I think they should match.

Starting from this example code.

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <ItemGroup>
     <_Item Include="One">
       <A></A>
     </_Item>
     <_Item Include="Two">
       <B></B>
     </_Item>
   </ItemGroup>
    <Target Name="MetadataValueEmpty">
      <Message Text="WithMetadataValueAEmpty: [@(_Item->WithMetadataValue('A', ''), '|')]"/>
      <Message Text="HasMetadataA: [@(_Item->HasMetadata('A'), '|')]"/>
      <!-- <Message Text="WithoutMetadataValueAEmpty: [@(_Item->WithoutMetadataValue('A', ''), '|')]"/> -->
      <Message Text="AnyHaveMetadataValueAEmpty: [@(_Item->AnyHaveMetadataValue('A', ''), '|')]"/>
      <Message Text="WithMetadataValueCEmpty: [@(_Item->WithMetadataValue('C', ''), '|')]"/>
      <Message Text="HasMetadataC: [@(_Item->HasMetadata('C'), '|')]"/>
      <!-- <Message Text="WithoutMetadataValueCEmpty: [@(_Item->WithoutMetadataValue('C', ''), '|')]"/> -->
      <Message Text="AnyHaveMetadataValueCEmpty: [@(_Item->AnyHaveMetadataValue('C', ''), '|')]"/>
      <Message Importance="high" Text="BatchConditionA: [@(_Item, '|')]" Condition=" '%(_Item.A)' == '' "/>
      <Message Importance="high" Text="BatchConditionC: [@(_Item, '|')]" Condition=" '%(_Item.C)' == '' "/>
   </Target>
 </Project>

Build as original MSBuild. The results are as following.

There is no metadata "C" in the above example. But WithMetadataValueCEmpty, AnyHaveMetadataValueCEmpty and BatchConditionC, their results return metadata "C" is existed.
For Metadata "A", HasMetaA returns nothing, but others conflict with HasMetadata and return that metadata "A" is existed. So, I think it's necessary to distinguish "undefined" and "defined but set to empty string".

  WithMetadataValueAEmpty: [One|Two]
  HasMetadataA: []
  AnyHaveMetadataValueAEmpty: [true]
  WithMetadataValueCEmpty: [One|Two]
  HasMetadataC: []
  AnyHaveMetadataValueCEmpty: [true]
  BatchConditionA: [One|Two]
  BatchConditionC: [One|Two]

After my changes. The outputs as following. But BatchCondition Condition=" '%(_Item.A)' == '' " should match with WithMetadataValue, that's an issue.

  WithMetadataValueAEmpty: [One]
  HasMetadataA: [One]
  AnyHaveMetadataValueAEmpty: [true]
  WithMetadataValueCEmpty: []
  HasMetadataC: []
  AnyHaveMetadataValueCEmpty: [false]
  BatchConditionA: [One|Two]
  BatchConditionC: [One|Two]

Forgind · 2023-04-24T15:28:32Z

Team triage:
We decided that adding a new way to distinguish between a metadatum that has been set at some point but is currently empty and a metadatum that was never set is not how we typically deal with undefined values in MSBuild and should not be added at this point. With that in mind, we want the WithoutMetadataValue function but would prefer to skip the other part of this PR.

JanKrivanek · 2023-06-02T12:12:56Z

@JaynieBai - do you plan to update this PR based on Forgind's last comment?

JaynieBai · 2023-06-05T06:49:54Z

do you plan to update this PR based on Forgind's last comment?

Thanks for your reminder. I missed that before. I will update this PR soon.

JaynieBai · 2023-06-12T08:30:40Z

Team triage:
We decided that adding a new way to distinguish between a metadatum that has been set at some point but is currently empty and a metadatum that was never set is not how we typically deal with undefined values in MSBuild and should not be added at this point. With that in mind, we want the WithoutMetadataValue function but would prefer to skip the other part of this PR.

Add the WithoutMetadataValue function in another PR #8867. So close this PR.

Distinguish present but empty and not present metadata of metadta ite…

7e94f3e

…m functions

JaynieBai marked this pull request as ready for review February 7, 2023 14:46

JaynieBai added 2 commits February 7, 2023 06:50

Remove unuseful comment

6d22f77

Add comments

acb8efd

Forgind approved these changes Feb 7, 2023

View reviewed changes

Comment thread src/Build/Evaluation/Expander.cs Outdated

Comment thread src/Build/Evaluation/Expander.cs Outdated

Forgind mentioned this pull request Feb 7, 2023

Distinguish present but empty and not present metadata in item functions #8408

Closed

JaynieBai added 3 commits February 7, 2023 23:57

introduce TryGetMetadataValueEscaped function

2f75325

Implememnt TryGetMetadataValueEscaped in project item

798e1be

Refactor the method

c0db5e3

ladipro reviewed Feb 13, 2023

View reviewed changes

JaynieBai added 2 commits February 13, 2023 23:35

Refactor the code 2

b99393e

return value

96b41a9

ladipro reviewed Feb 14, 2023

View reviewed changes

Add tests for new API

4d6a9bf

ladipro reviewed Feb 15, 2023

View reviewed changes

rainersigwald requested changes Feb 16, 2023

View reviewed changes

JaynieBai added 3 commits February 19, 2023 18:14

Merge branch 'main' of https://github.com/dotnet/msbuild into jennyba…

c5cf4e4

…i/issue8205

make the GetMetadataValueEscaped(string name, bool returnNullIfNotFou…

23f3b91

…nd) internal

Refactor function order

cec2cb2

rainersigwald modified the milestones: VS 17.6, VS 17.7 Mar 28, 2023

JaynieBai closed this Jun 12, 2023

JaynieBai deleted the jennybai/issue8205 branch June 12, 2023 08:30

	if (value == null) { return false; }
	else { return true; }
	return value != null;

	return GetMetadataValueEscaped(name, false);
	return GetMetadataValueEscaped(name, returnNullIfNotFound: false);

	public string GetMetadataValueEscaped(string name, bool returnNullIfNotFound)
	string IItem.GetMetadataValueEscaped(string name, bool returnNullIfNotFound)

Conversation

JaynieBai commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaynieBai Feb 14, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

JaynieBai commented Feb 17, 2023

Uh oh!

rainersigwald commented Feb 28, 2023

Uh oh!

JaynieBai commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Forgind commented Apr 24, 2023

Uh oh!

JanKrivanek commented Jun 2, 2023 • edited by Forgind Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaynieBai commented Jun 5, 2023

Uh oh!

JaynieBai commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JaynieBai commented Feb 7, 2023 •

edited

Loading

JaynieBai Feb 14, 2023 •

edited

Loading

JaynieBai commented Mar 3, 2023 •

edited

Loading

JanKrivanek commented Jun 2, 2023 •

edited by Forgind

Loading