Skip to content

AfterConvertCallback breaks the element order #777

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

Closed
adelabd opened this issue Aug 16, 2022 · 5 comments
Closed

AfterConvertCallback breaks the element order #777

adelabd opened this issue Aug 16, 2022 · 5 comments
Assignees
Labels
type: bug A general bug

Comments

@adelabd
Copy link

adelabd commented Aug 16, 2022

When we try to get a list of sorted entities, the order is lost if we use the EntityCallback.
I can confirm that the order is well respected in the AfterConvertCallback (I put a breakpoint in the onAfterConvert method and I can see each country sorted by countryName)
However, as the AfterConvertCallback is making some reactive call, the order is lost at the end and the method findAllByOrderByCountryName does not return an ordered flux anymore.

If I remove the EntityCallback, the sorting is well respected.

Entity country

public class Country {

    @Id
    private Long id;

    @NotNull
    private String countryName;
    
    @Transient
    private List<City> cities;
    
}

Country repository using ReactiveSorting

public interface CountryRepository  extends ReactiveSortingRepository<Country, Long> {
    Flux<Country> findAllByOrderByCountryName();
}
public interface CityRepository extends ReactiveCrudRepository<City, Long> {
    Flux<City> findAllByCountryId(Long countryId);
}
@Component
@AllArgsConstructor
public class CountryAfterConvert implements AfterConvertCallback<Country> {

    @Lazy
    private final CityRepository cityRepository;

    @Override
    public Publisher<Country> onAfterConvert(Country entity, SqlIdentifier table) {
        return cityRepository.findAllByCountryId(entity.getId())
                .collectList()
                .map(l -> {
                    entity.setCities(l);
                    return entity;
                });
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 16, 2022
@adelabd adelabd changed the title Reactive calls in AfterConvertCallback breaks ReactiveSortingRepository Reactive calls in AfterConvertCallback breaks the order from ReactiveSortingRepository Aug 16, 2022
@mp911de mp911de changed the title Reactive calls in AfterConvertCallback breaks the order from ReactiveSortingRepository AfterConvertCallback breaks the element order Aug 16, 2022
@mp911de mp911de self-assigned this Aug 16, 2022
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 16, 2022
@mp911de mp911de added this to the 1.5.3 (2021.2.3) milestone Aug 16, 2022
@mp911de
Copy link
Member

mp911de commented Aug 16, 2022

That's indeed a bug on our side as we use flatMap on Flux when applying the callback.

mp911de added a commit that referenced this issue Aug 16, 2022
We now use concatMap on result Fluxes to retain the object order. Previously, we used flatMap on a flux that has led to changes in element ordering.

Closes #777
@mp911de mp911de closed this as completed Aug 16, 2022
@adelabd
Copy link
Author

adelabd commented Aug 16, 2022

Maybe just using flatMapSequential should fix the problem.

@mp911de
Copy link
Member

mp911de commented Aug 16, 2022

concatMap is the right choice.

@adelabd
Copy link
Author

adelabd commented Aug 16, 2022

I don't think so.
concatMap => Generation of inners and subscription: this operator waits for one inner to complete before generating the next one and subscribing to it.

In this specific case, it's not required to wait the previous inner complete before subscribe to the new one.
We just need to keep the order at the end, not to be sequential during the execution of each item.
flatMapSequential seems to be more appropriate for this use case.

flatMapSequential => Transform the elements emitted by this Flux asynchronously into Publishers, then flatten these inner publishers into a single Flux, but merge them in the order of their source element.

@mp911de
Copy link
Member

mp911de commented Aug 16, 2022

With a flatMapSequential operator we would increase concurrency by number of stream elements * 16, and that impact is much more harmful than delaying subscription/completion. In general, callbacks on result streams aren't intended to create a N+1 problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants