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

Spring ORM SpringBeanContainer when trying to create a bean fails with not found bean definition, and fallbacks to default hibernate bean creation #30683

Closed
aurimasniekis opened this issue Jun 16, 2023 · 13 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@aurimasniekis
Copy link

Affected Version: 5.3.28

There seems to be an issue with Spring ORM SpringBeanContainer when Hibernate tries to create a bean using the SpringBeanContainer::createBean method:

SpringContainedBean<?> createBean(String name, Class<?> beanType, LifecycleOptions lifecycleOptions, BeanInstanceProducer fallbackProducer)

https://github.com/spring-projects/spring-framework/blob/main/spring-orm/src/main/java/org/springframework/orm/hibernate5/SpringBeanContainer.java#L183-L185

Upon the creation of a bean, the method this.beanFactory.applyBeanPropertyValues(bean, name); is called. This method internally attempts to merge the bean definition. However, the definition does not exist and cannot be created by the developer due to Hibernate appending a counter suffix to the bean name (e.g., org.example.types.CustomType0). This causes the entire process to fail with the error message "No bean named org.example.types.CustomType0," ultimately resorting to Hibernate's fallback method for bean creation. This negates the intended purpose of using the SpringBeanContainer for bean creation.

https://github.com/spring-projects/spring-framework/blob/main/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java#L403

A review of the code history suggests that this behavior has remained unchanged for quite some time. However, this approach appears to be flawed and may need to be reconsidered to ensure the proper functioning of the integration between Spring ORM and Hibernate when dealing with the creation of beans.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 16, 2023
@quaff
Copy link
Contributor

quaff commented Jun 17, 2023

Why a counter suffix will be appended to class name? IMO it's a bug that hibernate team should fix.

@aurimasniekis
Copy link
Author

Why a counter suffix will be appended to class name? IMO it's a bug that hibernate team should fix.

Because it's parameterized bean

@jhoeller jhoeller self-assigned this Jun 17, 2023
@jhoeller jhoeller added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Jun 17, 2023
@jhoeller
Copy link
Contributor

We could try to remove the suffix before considering this as a Spring bean name, but I really wonder why we get that name with a suffix there to begin with. Even for a parametermized bean, Hibernate should expose the raw bean name somewhere as well.

Is there a reliable indication for the provided bean name to be parameterized, so that we could strip the suffix for our purposes, but only in that specific case? Is this actually what you'd expect SpringBeanContainer to do, nevertheless finding a bean definition with name org.example.types.CustomType?

@aurimasniekis
Copy link
Author

I do not really think the issue is the suffix of the bean name. The issue is that the SpringBeanContainer creates a bean without any definition and then tries to merge the definition or etc, on nonexisting bean.

I have forked the SpringBeanContainer and replaced the createBean with way to create definition of bean and return it.

private SpringContainedBean<?> createBean(
        String name,
        Class<?> beanType,
        LifecycleOptions lifecycleOptions,
        BeanInstanceProducer fallbackProducer
) {
    try {
        if (lifecycleOptions.useJpaCompliantCreation()) {
            if (this.beanFactory instanceof BeanDefinitionRegistry bdr) {

                RootBeanDefinition beanDefinition = new RootBeanDefinition(beanType);

                bdr.registerBeanDefinition(name, beanDefinition);

                Object bean = this.beanFactory.getBean(name);

                return new SpringContainedBean<>(
                        bean,
                        beanInstance -> this.beanFactory.destroyBean(
                                name,
                                beanInstance
                        )
                );
            } else {
                throw new IllegalStateException(
                        "Cannot register bean definition with non-BeanDefinitionRegistry: "
                        + this.beanFactory);
            }
        } else {
            return new SpringContainedBean<>(this.beanFactory.getBean(
                    name,
                    beanType
            ));
        }
    } catch (BeansException ex) {

@aurimasniekis
Copy link
Author

If we still going around the suffix stuff here is the logic which causes the suffix addition when using custom types.

https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/mapping/BasicValue.java#L806-L866

@jhoeller
Copy link
Contributor

jhoeller commented Jun 17, 2023

It's not the lack of a bean definition, it's actually the presence of a provided bean name to begin with. We usually get callbacks for the BeanContainer.getBean(Class ...) variant, not for the variant with a bean name plus a Class. If a bean name is provided, we expect a pre-existing bean definition to get values from.

So in your case, it looks like we should essentially ignore the provided bean name and rather delegate to the getBean(Class ...) variant where no bean definition is expected to exist? Which makes me wonder why we receive a provided bean name here but not in other cases?

I guess we could check for the existence of a corresponding bean definition of the provided name and otherwise simply ignore the name.

@aurimasniekis
Copy link
Author

I see, this makes more sense right now, as far as I have done the research and some unrelated issues to the same code the suffix is appended because CustomType can be customized by parameters so that the same type would have multiple beans, not sure if that's the greatest way to do it.

@aurimasniekis
Copy link
Author

I think from my understanding the getBean with name doesn't really expect to get an already present bean like it's implemented but just the same as without name, but just a new instance of that type bean with name?

@jhoeller
Copy link
Contributor

Maybe that's why Hibernate provides a name there indeed, as a key for distinction between different bean instances. We can keep using the name as a key as we usually do there, but in the createBean(name, type) algorithm, we can seamlessly fall back to plain type-based creation if no bean definition of the given name exists. I'll implement it that way for 6.0.11 and 5.3.29.

@aurimasniekis
Copy link
Author

Going through other BeanContainer implementations it looks like my understanding is correct, Hibernate uses named getBean method to just create a bean with a different name for multiple instances of the same type bean.

hibernate/hibernate-orm@5460acd

@jhoeller
Copy link
Contributor

jhoeller commented Jun 17, 2023

The CDI BeanContainer implementation translates a given bean name to the jakarta.inject.Named annotation but I suppose that's not enforced in CDI, also finding a regular bean of that type if no bean with the specific name is found. So on our side, checking for the existence of a bean definition with the provided bean name and ignoring the name if not found seems to match that. As long as we keep using the provided name as a key for the bean instance.

Thanks for all the digging here, that's very helpful!

@jhoeller jhoeller added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 17, 2023
@jhoeller jhoeller added this to the 6.0.11 milestone Jun 17, 2023
@jhoeller jhoeller added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Jun 17, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x labels Jun 17, 2023
@aurimasniekis
Copy link
Author

Thanks for the fix! This will help for many stuff like JsonType and etc where we need to inject our instance of ObjectMapper which is customized a lot in the application.

jhoeller added a commit that referenced this issue Jun 17, 2023
@jhoeller
Copy link
Contributor

This is in 6.0.x/main as well as 5.3.x now, feel free to give it a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants