Implicit cyclical dependency about mapstruct-spring-extensions

[PRIESt512]

@ Lazy I added it to the code generation, I just hadn't tested it yet and was not sure about the beauty of the solution, this is a bit like a hack. However, if you also suggest such an option, then I will check the solution. Yes, I'll make an example that demonstrates this problem.

[Chessray]

Thank you. That's an interesting one. I noticed a weirdness myself, but hadn't gotten down to the bottom.

This means we might have to influence the format of the generated Mappers rather than only the adapter, so we'll require some assistance from the Mapstruct core. Tagging @filiphr and @sjaakd for visibility.

That being said, maybe we can resolve this inside the extensions project directly by adding a @Lazy to the injection point of the ConversionService into the generated adapter. @PRIESt512 Do you think you can provide a small example project demonstrating the issue?

[PRIESt512]

An example of a project with an error is here

А little later I will check the lazy generation)

[filiphr]

If you don't use constructor injection then this will work fine.

There are 2 other options that the Spring Extensions can do:

• Implement a ModelElementProcessor that will be executed just after the ModelElementProcessor and will look into the constructor and add the @Lazy to the appropriate field. This shouldn't be too hard by going through the Mapper#getConstructor and checking for AnnotatedConstructor, then going through the AnnotationMapperReference with a ListIterator and replacing the reference by calling AnnotationMapperReference#withNewAnnotations.
• Another approach would be to have the concept of XXXAware and similar like before with a ModelElementProcessor add adding an implementation of the XXXAware method.

Honestly, I personally am not a fan of the @Lazy annotation as sometimes it might lead to some inconsistencies. However, for this particular case, most likely it is OK.

[PRIESt512]

In the end, the reason for this strange behavior turned out to be spring. An BeanCurrentlyInCreationException was actually thrown inside it. But it was suppressed 🤦 This is due to a feature of the DefaultListableBeanFactory implementation. A piece of code for this ill-fated place:

			try {
				Object beanInstance = getBean(beanName);
				if (!(beanInstance instanceof NullBean)) {
					result.put(beanName, (T) beanInstance);
				}
			}
			catch (BeanCreationException ex) {
				Throwable rootCause = ex.getMostSpecificCause();
				if (rootCause instanceof BeanCurrentlyInCreationException) {
					BeanCreationException bce = (BeanCreationException) rootCause;
					String exBeanName = bce.getBeanName();
					if (exBeanName != null && isCurrentlyInCreation(exBeanName)) {
						if (logger.isTraceEnabled()) {
							logger.trace("Ignoring match to currently created bean '" + exBeanName + "': " +
									ex.getMessage());
						}
						onSuppressedException(ex);
						// Ignore: indicates a circular reference when autowiring constructors.
						// We want to find matches other than the currently created bean itself.
						continue;
					}
				}
				throw ex;
			}

In fact, this is a clear cyclical dependence that has been rudely suppressed. I'm sitting here and thinking if this is a bug or a feature 🤪

Moreover, on older versions of the spring, with a large number of converters, this could potentially lead to OutOfMemory due to the large number of exceptions.
Each unsuccessful injection attempt is one entry in the collection)))

[danielshiplett]

I believe that the correct solution, according to the Spring documentation is to add a setter that is marked @Autowired. This will allow the constructor call to proceed without the cyclical dependency. Then, once the ConversionServiceAdapter is initialized, Spring will come back and call the Autowired setter.

So in short, remove the generation of the Autowired private field (leave the field, remove the Autowired). Then add:

@Autowired
public void setConversionServiceAdapter(ConversionServiceAdapter conversionServiceAdapter) {
  this.conversionServiceAdapter = conversionServiceAdapter;
}

This avoids the need for @lazy which can cause applications to start successfully even when a required Bean is missing. In this case the lazy will probably be OK as this is all generated code and nothing should be missing.

[woodforda]

I had some issues with Converters not being found due to start up order. worked around this by setting the @AutoConfigureOrder(Ordered.HIGHEST_PRECEDENCE + 50) of where my converters are defined (or component scanned). This makes them available before the Boot tries to pick them up for the ConversionService. Obviously, if the converter depends on other beans then this would force earlier instantiation of them too so it's not always viable.

Also inject an ObjectProvider which I can use to lazily retrieve the service (and still keep constructor injection) when (if) the situation requires.

[Chessray]

I believe that the correct solution, according to the Spring documentation is to add a setter that is marked @Autowired. This will allow the constructor call to proceed without the cyclical dependency. Then, once the ConversionServiceAdapter is initialized, Spring will come back and call the Autowired setter.

So in short, remove the generation of the Autowired private field (leave the field, remove the Autowired). Then add:

@Autowired
public void setConversionServiceAdapter(ConversionServiceAdapter conversionServiceAdapter) {
  this.conversionServiceAdapter = conversionServiceAdapter;
}

This avoids the need for @lazy which can cause applications to start successfully even when a required Bean is missing. In this case the lazy will probably be OK as this is all generated code and nothing should be missing.

I agree that the @Autowired setter feels somewhat cleaner than @Lazy. However, this is in the Mapper itself. At this point, I'm a bit reluctant to modify the mapper generation from the extension. We'll probably go ahead with @Lazy if we can include the test example. I'm hoping to find a little time for this over the next couple of days.

[danielshiplett]

Yeah. After I posted by message, I dug into the code and realized that it was core MapStruct and not the extension that would need the change. Oh well. I took the proposed MR and tested it and it worked fine. So I'm OK with the @lazy annotation being added.