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

AssertFactory with the ability to convert the value upfront #3368

Closed
snicoll opened this issue Feb 18, 2024 · 24 comments · Fixed by #3377
Closed

AssertFactory with the ability to convert the value upfront #3368

snicoll opened this issue Feb 18, 2024 · 24 comments · Fixed by #3377
Assignees
Labels
type: new feature A new feature

Comments

@snicoll
Copy link

snicoll commented Feb 18, 2024

Feature summary

While working on the AssertJ support for Spring MVC, I've realized that assertJ does not provide a lot of conversion utilities for the actual value, which is more than fair, but can be helpful higher in the stack.

One of the support we have is for json path where you can craft an expression against the body result of an HTTP request and further assert the content. Our json path feature an "extractingPath" that is similar to AssertJ's extracting feature. But that operates on the raw value and we want to be able to assert that the raw JSON can be deserialized to a DTO.

Example

Let's take a silly little example with a family API that returns this:

{
  "familyMembers": [
    { "name": "Homer" },
    { "name": "Marge" },
    { "name": "Bart" },
    { "name": "Lisa" },
    { "name": "Maggie" }
  ]
}

Ignoring the setup, this is the pseudo code I am talking about:

@Test
void convertToTargetType() {
    assertThat(mvc.perform(get("/family"))).body().jsonPath()
            .extractingPath("$.familyMembers[0]").convertTo(Member.class)
            .satisfies(member -> assertThat(member.name).isEqualTo("Homer"));
}

This works but I am returning an AbstractObjectAssert against my DTO that represent a family member. What's I'd like to do is to express a type and, at the same time, provide a dedicated Assert implementation for it.

InstanceOfAssertFactory does what I need but getType is package private so I can't access it. Also I like the idea of having an interface for this concept and expose a Type so that generic types can be taken into account perhaps?

Also InstanceOfAssertFactorycarry the fact that the actual is expected to be of that type which, again, is totally fair but custom APIs may benefit from conversion as well.

@scordio
Copy link
Member

scordio commented Feb 18, 2024

Could you help me understand your example better by adding which method returns (or would return) what? E.g.:

assertThat(mvc.perform(get("/family"))) // dedicated Assert 
        .body().jsonPath()
        .extractingPath("$.familyMembers[0]").convertTo(Member.class) // AbstractObjectAssert
        .satisfies(member -> assertThat(member.name).isEqualTo("Homer"));

or feel free to point me to an example in Spring MVC, if there is any.

InstanceOfAssertFactory does what I need but getType is package private so I can't access it.

I kept it package-private because I couldn't imagine a use case for having it exposed, but happy to evaluate it again. Could you elaborate on how getType() would be consumed in the use case you have in mind?

@snicoll
Copy link
Author

snicoll commented Feb 18, 2024

Could you help me understand your example better by adding which method returns (or would return) what? E.g.:

extractingPath returns an AbstractObjectAssert extension whose actual is the raw json value resolved by the given path. In the case above, this is a Map with a name attribute equals to Homer. We can use that as a source to rebuild the actual message.

convertTo(Class<T>) does the conversion and returns an AbstractObjectAssert<?, T>.

You can also see a (different) example in the branch I am working on.

Could you elaborate on how getType() would be consumed in the use case you have in mind?

If getType or anything that lets me know the type would be public I could then ask Spring's converter "please convert the input into that type. And once that is done, I can use the assertion object to get a dedicated object. The problem currently is that even if you provide a type that assertJ supports natively you always get an ObjectAssert of T which forces you to use satisfies if you want to get back dedicated assertions.

@scordio
Copy link
Member

scordio commented Feb 18, 2024

The problem currently is that even if you provide a type that assertJ supports natively you always get an ObjectAssert of T which forces you to use satisfies if you want to get back dedicated assertions.

That's exactly why I introduced an InstanceOfAssertFactory parameter instead of a Class one for asInstanceOf and the extracting variants, basically due to type erasure. Conceptually, the InstanceOfAssertFactory instance maps the expected type to the corresponding Assert subtype, information that the compiler can use to allow type-specific assertions.

Do I understand correctly that you would like convertTo(Class<T>) to return type-specific assertions depending on its Class parameter? How would you solve the type erasure issue?

@scordio
Copy link
Member

scordio commented Feb 18, 2024

Looking at your branch, specifically:

public <T> AbstractObjectAssert<?, T> convertTo(Class<T> target) {
	isNotNull();
	T value = convertToTargetType(target);
	return Assertions.assertThat(value);
}

due to type erasure, the compiler cannot identify a "better" candidate than assertThat(T), which is used as a fallback since T is unbounded.

To get type-specific assertions, the client code would need to specify the desired assertion in one way or another, similar to the asInstanceOf examples.

Or did I understand something totally different?

@scordio
Copy link
Member

scordio commented Feb 18, 2024

Now I think I understood what you mean. If convertTo would evolve as:

public <ASSERT extends AbstractAssert<?, ?>> ASSERT convertTo(InstanceOfAssertFactory<?, ASSERT> factory) {
  // `isNotNull()` implicitly done by `extracting`
  return extracting(__ -> convertToTargetType(targetType), factory); // where to get `targetType`?
}

as of today, there is no way to get targetType without an additional parameter.

Is that what you're looking for?

@joel-costigliola
Copy link
Member

@snicoll my 2cts here is: since you are dealing with JSON would a JSON assertion library be a better fit than AssertJ? I'm thinking of https://github.com/lukas-krecan/JsonUnit for example

@scordio
Copy link
Member

scordio commented Feb 18, 2024

If I got @snicoll correctly, the pain point is actually after JSON handling and I can imagine it would circle back to AssertJ even with JsonUnit.

Also, I guess it's related to spring-projects/spring-framework#21178.

@snicoll
Copy link
Author

snicoll commented Feb 19, 2024

@scordio I've been going back and forth on my branch, but here is something that looks a bit like what I was after. It's far from perfect but I believe it should help understand the context.

due to type erasure, the compiler cannot identify a "better" candidate than assertThat(T), which is used as a fallback since T is unbounded.

In Spring Framework we have ParameterizedTypeReference that lets you provide T with generic information that works ultimately on a java.lang.reflect.Type. ValueType in this commit is already an InstanceOfAssertFactory but keeps the Type reference for doing the conversion. Later I've removed that class altogether as I wanted to get some feedback from you about the feature first.

As for checking if actual is indeed of type Type, there's a "shortcut" on doing an instanceOf on the raw type so this is definitely far from perfect. The actual conversion is responsible to figure out if it can produce the actual requested type. All that happens outside of AssertJ so I was trying to figure out something that is as minimal as possible that lets me reuse standard constructs.

I was wondering if I could get away with a default method on AssertFactory that I could call, something along the lines of:

default ASSERT createAssert(Object actual, Function<Type, T> conversionFactory) {
    T value = conversionFactory.apply(theType);
    return createAssert(value);
}

The problem now being, of course, that the existing implementations in InstanceOfAssertFactories only have the raw type...

@joel-costigliola we already have JSON Path and JSON Assert support in the Spring Framework that I am trying to integrate with AssertJ. One request was to be able to run assertions on a high-level DTO, rather that the raw JSON value.

@scordio
Copy link
Member

scordio commented Feb 20, 2024

@snicoll I'm going back and forth through your changes, trying to understand how AssertJ might help.

If you would put aside backward compatibility, what change or feature would you like to see in AssertJ to support your use case better?

Is it summarized by what you initially wrote?

I like the idea of having an interface for this concept and expose a Type so that generic types can be taken into account perhaps?

Also, about:

assertJ does not provide a lot of conversion utilities for the actual value, which is more than fair, but can be helpful higher in the stack.

I'm still unsure if we should add conversion utilities to AssertFactory, like the example in your previous message.

Should we maybe explore this direction only if we cannot come up with something better in the InstanceOfAssertFactory area? Or is it something that you would need anyway?

@snicoll
Copy link
Author

snicoll commented Feb 20, 2024

I'm still unsure if we should add conversion utilities to AssertFactory, like the example in your previous message.

I don't think we should, but offering a way using an opt-in method would certainly be very much appreciated. In my example, AssertJ does not add support for conversion, but rather allow a callback so that something else can.

To summarize my ask. AssertJ already has a number of out-ot-the-box features to represent various types that provides a dedicated assert object. InstanceOfAssertFactories is the main one I have in mind. But this class works on the assumption that actual is of that type. It's ok and the current createAssert is totally fine. I am asking about another version where someone else could get a chance to convert the value before the assert object is created.

If the feature is declined then, I have two choices:

  1. Do nothing and then users get a generic assert object (so in the case you ask to convert to a List<Member> you get an AbstractObjectAssert<List<Member>>)
  2. Creating my own type, which means that I have to redo what InstanceOfAssertFactories does.

@scordio
Copy link
Member

scordio commented Feb 20, 2024

I don't see why we should decline this request 🙂

Let me digest it a bit better and come back with a proposal.

@scordio
Copy link
Member

scordio commented Feb 21, 2024

@snicoll could you point me to a test case that cannot work today because of the raw type usage in InstanceOfAssertFactories?

@snicoll
Copy link
Author

snicoll commented Feb 21, 2024

Sure, I should have done that from the very beginning:
https://github.com/snicoll-scratches/assertj-conversion

The example is contrived to show the status quo and what I would like to achieve. With InstanceOfAssertFactory, it can only be used when actual is of the requested type. ValueType in the project shows how it can be augmented to offer opt-in conversion.

The test case does not use any generics type but I can add another test if needs to be.

I am not aware that assertJ has a way to refer to a type with generic information, such as the one from Spring Framework I am using there so it's more than just the extra method I've shared above. That said, with InstanceOfAssertFactories providing shortcut for a number of cases, it could be easier to build that internally.

I should mention that I am happy to contribute based on your guidance if that can help.

@scordio
Copy link
Member

scordio commented Feb 21, 2024

I should mention that I am happy to contribute based on your guidance if that can help.

Very much appreciated 🙂 I keep digging to get a clearer idea of where we want to go and then we can discuss who will take up the changes!

@scordio
Copy link
Member

scordio commented Feb 21, 2024

I am not aware that assertJ has a way to refer to a type with generic information, such as the one from Spring Framework I am using there so it's more than just the extra method I've shared above.

This is indeed a direction I'd like to investigate. I always had the impression that InstanceOfAssertFactory instances like list(Class) were a bit "weak"... maybe adding some support to types with generic information could be a way to go but I assume it should be somehow compatible with what Spring already offers.

@snicoll
Copy link
Author

snicoll commented Feb 21, 2024

I assume it should be somehow compatible with what Spring already offers.

Nope. You need to be able to build a java.lang.reflect.Type programmatically with the relevant generic information. That's what Spring's ResolvableType does but we only need to Type information of the JDK. If you look at ParameterizedTypeReference in Spring, you can see another "cheap" way of doing this and get a Type.

@scordio
Copy link
Member

scordio commented Feb 23, 2024

The test case does not use any generics type but I can add another test if needs to be.

@snicoll I'm sketching a possible solution so it would help a lot to see a more complex case in your repo, if you have time for it.

@snicoll
Copy link
Author

snicoll commented Feb 23, 2024

What do you need? A conversion for a type with generics?

@scordio
Copy link
Member

scordio commented Feb 23, 2024

Yes, just to make sure I'm not composing something incompatible with what you would expect from users.

@snicoll
Copy link
Author

snicoll commented Feb 23, 2024

I've added a commit (that does not compile) as providing the type and then a function to get the assertObject is not straightforward. However, relying on the existing built-in instances would be already very good so I don't know if that's a blocker.

@scordio
Copy link
Member

scordio commented Feb 23, 2024

The direction I'm evaluating is without an opt-in method for value conversion, but making sure that the consumer code can apply arbitrary conversions before the InstanceOfAssertFactory instance is invoked.

Assuming that InstanceOfAssertFactory offers a new Type getType() method (or, even better, there is an interface for it), to address:

@Test
void convertToUsingInstanceOfAssertFactory() {
	assertThat(forValue(123)).convertTo(InstanceOfAssertFactories.STRING).startsWith("1");
}

and

@Test
void convertToListOfObject() {
	Customer[] customers = new Customer[] { new Customer("123", "John", "Smith", 42) };

	assertThat(forValue(customers)).convertTo(InstanceOfAssertFactories.list(Customer.class))
	.hasSize(1).contains(new Customer("123", "John", "Smith", 42));
}

CustomAssert could become:

public class CustomAssert extends AbstractObjectAssert<CustomAssert, Object> {

	public CustomAssert(Object actual) {
		super(actual, CustomAssert.class);
	}

	public <T, ASSERT extends AbstractAssert<?, ?>> ASSERT convertTo(InstanceOfAssertFactory<T, ASSERT> factory) {
		return new ConversionServiceBasedAssertFactory<>(factory).createAssert(this.actual);
	}


	private static class ConversionServiceBasedAssertFactory<T, ASSERT extends AbstractAssert<?, ?>> implements AssertFactory<Object, ASSERT> {

		// FIXME should be an interface instead of InstanceOfAssertFactory
		private final InstanceOfAssertFactory<T, ASSERT> delegate;
		private final ResolvableType resolvableType;

		private ConversionServiceBasedAssertFactory(InstanceOfAssertFactory<T, ASSERT> delegate) {
			this.delegate = delegate;
			this.resolvableType = ResolvableType.forType(List.class); // FIXME should be delegate.getType()
		}

		@Override
		public ASSERT createAssert(Object actual) {
			return delegate.createAssert(convert(actual));
		}

		@SuppressWarnings("unchecked")
		private T convert(Object actual) {
			Object convert = DefaultConversionService.getSharedInstance().convert(
					actual, new TypeDescriptor(resolvableType, null, null));
			return (T) convert;
		}

	}

}

This obviously is not enough for the third case you introduced:

@Test
void convertToListOfObjectUsingGenericType() {
	Customer[] customers = new Customer[] { new Customer("123", "John", "Smith", 42) };

	assertThat(forValue(customers)).convertTo(/* something coming from AssertJ that supports parameterized type references? */)
		.hasSize(1).contains(new Customer("123", "John", "Smith", 42));
}

If it can solve this use case, I'm definitely keen to introduce something similar to ParameterizedTypeReference in AssertJ. I'll continue digging and keep you updated.

@scordio scordio self-assigned this Feb 23, 2024
@snicoll
Copy link
Author

snicoll commented Feb 23, 2024

Assuming that InstanceOfAssertFactory offers a new Type getType() method (or, even better, there is an interface for it), to address:

Having that would help with the use case we're trying to implement.

This obviously is not enough for the third case you introduced:

It does not matter, I think. I was trying to show how to use ParameterizedType but if you need to do something that InstanceOfAssertFactories does not provide already, then you'd probably better off creating a method anyway.

@scordio
Copy link
Member

scordio commented Feb 24, 2024

My wish would be to at least enhance factories like list(Class).

As you also pointed out, today these factories know the raw type only. Ideally, getType() should return the corresponding ParameterizedType instance instead. So far, it seems feasible in my draft but I'm currently keeping the AssertJ version of ParameterizedTypeReference as a package-private detail.

However, would it make sense to have it public and also add new factories like list(ParameterizedTypeReference)?

This would allow users to write things like:

Object actual = List.of(Set.of("a", "b"), Set.of("c", "d"));

ParameterizedTypeReference<Set<String>> elementTypeReference = new ParameterizedTypeReference<>() {};

assertThat(actual).asInstanceOf(list(elementTypeReference)).hasSize(2);

where the a getType() call on list(elementTypeReference) would return the proper List<Set<String>> instance of ParameterizedType.

@scordio
Copy link
Member

scordio commented Feb 24, 2024

BTW over the weekend I'll polish what I have and push it so you can have a better idea of where I'm headed.

scordio added a commit that referenced this issue Mar 5, 2024
scordio added a commit that referenced this issue Mar 24, 2024
# Conflicts:
#	assertj-core/src/test/java/org/assertj/core/api/InstanceOfAssertFactoriesTest.java
scordio added a commit that referenced this issue Mar 24, 2024
# Conflicts:
#	assertj-core/src/main/java/org/assertj/core/api/InstanceOfAssertFactories.java
#	assertj-core/src/test/java/org/assertj/core/api/InstanceOfAssertFactoriesTest.java
@scordio scordio added the type: new feature A new feature label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: new feature A new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants