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

Add feature to check if an XElement or XAttribute is absent within the XDocument #2589

Open
skukshaus opened this issue Feb 25, 2024 · 25 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation enhancement

Comments

@skukshaus
Copy link

skukshaus commented Feb 25, 2024

Background and motivation

Currently, there is an implementation to check whenever an Attribute or Element is within the Document: https://fluentassertions.com/xml/
However, sometimes it is necessary to check if those attributes are absent. Currently, there is no possibility to do this. Maybe it would be possible to use the HaveElement() Method and catch the Exception, but this feels dirty.

I think it could also be a good idea, to add a constraint if the attribute/element does not have a specific value

API Proposal

public class XElementAssertions
{
    public AndConstraint<XElementAssertions> NotHaveAttribute(string unexpectedAttribute, string because = "", params object[] becauseArgs);
    public AndConstraint<XElementAssertions> NotHaveAttribute(string unexpectedAttribute, string unexpectedValue, string because = "", params object[] becauseArgs);

    public AndWhichConstraint<XElementAssertions, XElement> NotHaveElement(string unexpectedElement, string because = "", params object[] becauseArgs);
    public AndWhichConstraint<XElementAssertions, XElement> NotHaveElement(string unexpectedElement, object unexpectedValue, string because = "", params object[] becauseArgs);
}

API Usage

var element = XElement.Parse("<user><forename>john</forename></user>");
element.Should().NotHaveElement("surname");


var element = XElement.Parse("<user><forename>john</forename><surname>doe</surname></user>");
element.Should().NotHaveElement("surname", "smith");



var element = XElement.Parse("<user forename='john' />");
element.Should().NotHaveAttribute("surname");


var element = XElement.Parse("<user forename='john' surname='doe' />");
element.Should().NotHaveAttribute("surname", "smith");

Alternative Designs

No response

Risks

No response

Are you willing to help with a proof-of-concept (as PR in that or a separate repo) first and as pull-request later on?

Yes, please assign this issue to me.

@skukshaus skukshaus added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 25, 2024
@dennisdoomen
Copy link
Member

Sure, makes sense. What do you think @jnyrup ?

@dennisdoomen dennisdoomen changed the title [API Proposal]: Add feature, to check if an XElement or XAttribute is absent within the XDocument Add feature to check if an XElement or XAttribute is absent within the XDocument Mar 15, 2024
@jnyrup
Copy link
Member

jnyrup commented Mar 24, 2024

It certainly makes sense to have more negated assertions for XML types 👍

You suggest adding an overload of NotHaveElement having an object unexpectedValue, but we don't currently have an affirmative HaveElement(string expectedElement, object expectedValue).
So I'm not sure we should add that overload.

We currently have HaveElement(string expectedName) on

  • XDocumentAssertions
  • XElementAssertions
  • XmlElementAssertions

and HaveAttribute(string expectedName, string expectedValue) on

  • XElementAssertions
  • XmlElementAssertions

In order to have a consistent API, it would be be nice to add the proposed methods on all relevant classes.

@skukshaus
Copy link
Author

skukshaus commented Mar 26, 2024

Hey @jnyrup and @dennisdoomen

Thanks for the reply, should I implement the changes according to your guidelines and create another PR?
I mean, including the overloads for the other use cases as well.

(I mean, I really need this extension because I have a dirty workaround for my problem (I've added a Must<XElementAssertions>() method because Should<XElementAssertions>() is already there).)

BG Sergej

@dennisdoomen
Copy link
Member

Thanks for the reply, should I implement the changes according to your guidelines and create another PR?

Everything that is in this proposal should be delivered as a consistent set of changes. And to prevent contributors from abandoning the work after the first PR, we need to insist on contributors to provide a PR that covers everything that is needed to maintain that consistency. Hope you understand.

@dennisdoomen
Copy link
Member

It took me a couple of seconds to realize that element.Should().NotHaveElement("surname", "smith"); meant that element should not have an element with a specific value. I guess that also means that if element has element surname with a different value, it is okay? If so, then I think we should call it NotHaveElementWithValue.

In that case, we need:

  • XDocumentAssertions.HaveElementWithValue

  • XDocumentAssertions.NotHaveElementWithValue

  • XDocumentAssertions.NotHaveElement

  • XElementAssertions.HaveElementWithValue

  • XElementAssertions.NotHaveElementWithValue

  • XElementAssertions.NotHaveElement

  • XElementAssertions.HaveAttributeWithValue

  • XElementAssertions.NotHaveAttributeWithValue

  • XElementAssertions.NotHaveAttribute

To be clear, you can only add the NotHaveElementWithValue if you also include HaveElementWithValue

XmlElementAssertions are out of scope as far as I'm concerned.

@dennisdoomen
Copy link
Member

@jnyrup @skukshaus any thoughts?

@skukshaus
Copy link
Author

Sorry for the delay, Dennis,
I am a bit busy currently, so I was unable to complete the request at the moment.
BG Sergej

@jnyrup
Copy link
Member

jnyrup commented May 15, 2024

We currently have XmlElementAssertions.HaveAttribute(string expectedName, string expectedValue) and XElementAssertions.HaveAttribute(string expectedName, string expectedValue).
In hindsight these should probably had been named HaveAttributeWithValue to distinguish them from asserting whether an element has an attribute with any value.

To avoid scope creeping, I suggest we focus on NotHaveElement for now to progress towards an API approval.

public class XDocumentAssertions
{
    public AndConstraint<XDocumentAssertions> NotHaveElement(string unexpected, string because = "", params object[] becauseArgs)
    public AndConstraint<XDocumentAssertions> NotHaveElement(XName unexpected, string because = "", params object[] becauseArgs)
}

public class XElementAssertions
{
    public AndConstraint<XElementAssertions> NotHaveElement(string unexpected, string because = "", params object[] becauseArgs)
    public AndConstraint<XElementAssertions> NotHaveElement(XName unexpected, string because = "", params object[] becauseArgs)
}

public class XmlElementAssertions
{
    public AndConstraint<XmlElementAssertions> NotHaveElement(string unexpectedName, string because = "", params object[] becauseArgs)
}

@dennisdoomen
Copy link
Member

I'd rather use this opportunity to correct the situation.

@leus
Copy link

leus commented May 16, 2024

Hey, I need this also - willing to help if you need an extra pair of hands.

@jnyrup
Copy link
Member

jnyrup commented May 17, 2024

@leus as we're discussing several methods, which one(s) are you looking for?

@jnyrup
Copy link
Member

jnyrup commented May 17, 2024

I'd rather use this opportunity to correct the situation.

Then I guess we should have:

  • HaveAttribute(string expectedName)
    • The attribute exists and can have any value
  • HaveAttributeWithValue(string expectedName, string expectedValue)
    • The attribute exists and has a particular value
  • NotHaveAttribute(string unexpectedName)
    • The attribute does not exist
  • NotHaveAttributeWithValue(string unexpectedName, string unexpectedValue)
    • The attribute either does not exists or doesn't have the particular value

@eNeRGy164
Copy link
Contributor

eNeRGy164 commented May 17, 2024

I think the last one is a bit ambiguous. I would opt for NotHaveAttributeValue and to require the attribute itself actually exist.
(Which would make HaveAttributeValue the opposite)

@dennisdoomen
Copy link
Member

I'd rather use this opportunity to correct the situation.

Then I guess we should have:

  • HaveAttribute(string expectedName)

    • The attribute exists and can have any value
  • HaveAttributeWithValue(string expectedName, string expectedValue)

    • The attribute exists and has a particular value
  • NotHaveAttribute(string unexpectedName)

    • The attribute does not exist
  • NotHaveAttributeWithValue(string unexpectedName, string unexpectedValue)

    • The attribute either does not exists or doesn't have the particular value

Yes, on XDocumentAssertions and XElementAssertions. I would skip XmlElementAssertions

I think the last one is a bit ambiguous. I would opt for NotHaveAttributeValue and to require the attribute itself actually exist.
(Which would make HaveAttributeValue the opposite)

ChatGPT believes that the behavior suggested by @jnyrup makes more sense. I tend to agree.

@dennisdoomen
Copy link
Member

Hey, I need this also - willing to help if you need an extra pair of hands.

@leus I'll ping you when we have settled on the API.

@leus
Copy link

leus commented May 17, 2024

Sure - in the meantime I'm using node.Element(something).Should().BeNull() (or something like that, I don'r remember) in my tests and it's not too bad but really bothers me. Looking forward to help.

@jnyrup
Copy link
Member

jnyrup commented May 18, 2024

I think the last one is a bit ambiguous. I would opt for NotHaveAttributeValue and to require the attribute itself actually exist. (Which would make HaveAttributeValue the opposite)

If we let p be "having the attribute" and q "with the given value", then the negation is !(p && q) and by applying De Morgan's Law we !q || !p, or in prose "not having the attribute or not having the given value".

Asserting that some has an attribute but not with a given value would be HaveAttributeWithoutValue.

To avoid that confusion, we could e.g. choose to not expose either but let the developer write the unambigious:

foo.Should().HaveAttribute(bar)
    .Which.Value.Should().NotBe(baz);

I would skip XmlElementAssertions

Why?

@dennisdoomen
Copy link
Member

If we let p be "having the attribute" and q "with the given value", then the negation is !(p && q) and by applying De Morgan's Law we !q || !p, or in prose "not having the attribute or not having the given value".

Although I've learned something new again (thank you 😜), such formal definitions don't mean much for must developers. In the end it's about intuitivity.

To avoid that confusion, we could e.g. choose to not expose either but let the developer write the unambigious:

I like that.

Why?

To reduce the scope of this PR and because XmlElement are ancient. I haven't used those since we have XDocument.

@cbersch
Copy link

cbersch commented May 27, 2024

How could we check if an attribute is missing or set to ""?
This is often treated equally, but cannot be modeled by the proposed methods.

I would expect NotHaveAttribute to not handle that case:

XElement.Parse("<root attr=\"\" />").Should().NotHaveAttribute("attr");
// -> succeeds

So we would need an extension like HaveNullOrEmptyAttribute(string unexpected), but I don't really like that name, because it starts with Have but is basically a NotHave.

@jnyrup
Copy link
Member

jnyrup commented May 27, 2024

If we let p be "having the attribute" and q "with the given value", then the negation is !(p && q) and by applying De Morgan's Law we !q || !p, or in prose "not having the attribute or not having the given value".

Although I've learned something new again (thank you 😜), such formal definitions don't mean much for must developers. In the end it's about intuitivity.

In #2321 we changed our code to behave more logically, because our intuition failed to imagine how developers would use the API.

I agree that logic is not always immediate intuitive and I don't mean to force all developers to know De Morgan's law by heart, but I would conjecture that looking back at the past thousands of years that logic has a better track record than intuition.
I can't speak for how most developers think, but my first suggestion was also what I found the intuitive.
and so should hopefully all developers with a CS background as this topic is literally the first course I had.

Thinking more about it, here's an example of where I would say the logically correct version of NotHaveAttributeWithValue also does what one would expect and cannot be easily be expressed as a combination of other APIs.

Should fail:

var subject = XElement.Parse("<root error=\"myErrorType\" />");
subject.Should().NotHaveAttributeWithValue("error", "myErrorType");

Both of these should pass because they either have another error value or no error value at all.

var subject = XElement.Parse("<root error=\"someOthererrorType\" />");
subject.Should().NotHaveAttributeWithValue("error", "errorType");

var subject = XElement.Parse("<root />");
subject.Should().NotHaveAttributeWithValue("error", "errorType");

Why?

To reduce the scope of this PR and because XmlElement are ancient. I haven't used those since we have XDocument.

Then I suggest that after approving an API for XDocument and XElement, we perhaps create a separate issue for XmlElement to get a more consistent API.

@dennisdoomen
Copy link
Member

I agree that logic is not always immediate intuitive and I don't mean to force all developers to know De Morgan's law by heart, but I would conjecture that looking back at the past thousands of years that logic has a better track record than intuition.
I can't speak for how most developers think, but my first suggestion was also what I found the intuitive.
and so should hopefully all developers with a CS background as this topic is literally the first course I had.

For me that has been almost 30 years ago. I'm already happy that I sometimes still remember what my wife said yesterday ;-)

@dennisdoomen
Copy link
Member

I think the last one is a bit ambiguous. I would opt for NotHaveAttributeValue and to require the attribute itself actually exist.
(Which would make HaveAttributeValue the opposite)

Actually, I would expect this one to have a single parameter and mean that whatever attribute the element has, it should not have that value.

@dennisdoomen
Copy link
Member

How could we check if an attribute is missing or set to ""?
This is often treated equally, but cannot be modeled by the proposed methods.

That's a completely different situation. And I don't believe an attribute with an empty string is the same. In fact, it isn't.

@dennisdoomen
Copy link
Member

Which brings us back to the (hopefully) final proposal:

  • HaveElementWithValue(string expectedName, string expectedValue) - The element exists and has a particular value
  • NotHaveElement(string unexpectedName) - The element does not exist
  • NotHaveElementWithValue(string unexpectedName, string unexpectedValue) - The element either does not exist or doesn't have the particular value
  • HaveAttribute(string expectedName) - The attribute exists and can have any value
  • HaveAttributeWithValue(string expectedName, string expectedValue) - The attribute exists and has a particular value
  • NotHaveAttribute(string unexpectedName) - The attribute does not exist
  • NotHaveAttributeWithValue(string unexpectedName, string unexpectedValue) - The attribute either does not exist or doesn't have the particular value

Plus overloads where it takes an XName instead of a string to find an element

@cbersch
Copy link

cbersch commented Jun 3, 2024

In your final proposal the handling of "null or empty attribute" isn't covered, see #2589 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation enhancement
Projects
None yet
Development

No branches or pull requests

6 participants