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

Converter called multiple times evaluating aggregation operation query methods #4712

Closed
MrWangGang opened this issue May 31, 2024 · 5 comments
Assignees
Labels
type: bug A general bug

Comments

@MrWangGang
Copy link

my Converter

@WritingConverter
public class EnumWriteConverter implements Converter<ConverterEnum, String> {
    public EnumWriteConverter() {
    }

    public String convert(ConverterEnum source) {
        return source.getValue();
    }
}

Pass the enumeration as a parameter,my enum impl -> ConverterEnum


    @Aggregation({
            "{$addFields: { '_idString': { $toString: '$_id' } }}",
            "{$match: { userId: ?0 }}",
            "{$lookup: { from: 't_sku_skill', localField: '_idString', foreignField: 'itemId', as: 'skuDetails' }}",
            "{$unwind: { path: '$skuDetails', preserveNullAndEmptyArrays: true }}",
            "{$match: { deletedEnum: ?1 }}",
            "{$sort: { availableEnum: 1 }}", // ASC
            "{$addFields: { quantity: '$skuDetails.quantity' }}",
            "{$project: { _idString: 0, skuDetails: 0 }}"
    })
    Flux<ItemSkillPO.AggregationSku> findBy(String userId, EnumCommerceItemDeletedEnum deleted , Pageable pageable);

"I found that the Converter was called multiple times, and the source was always the same enumeration, ENUM_COMMERCE_ITEM_DELETED_UNDELETE, because I called this query method in the business logic."

   @Override
    public Flux<ItemSkillPO.AggregationSku> page(String userId, Pageable pageable) {
        return repository.findBy(userId,ENUM_COMMERCE_ITEM_DELETED_UNDELETE,pageable);
    }

I only called the query method once, but the Converter was called multiple times. Why?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 31, 2024
@MrWangGang
Copy link
Author

@christophstrobl
Copy link
Member

Thank you for reaching out - If you'd like us to spend some time investigating, please take the time to provide a complete minimal sample (something that we can unzip or git clone, build, and deploy) that reproduces the problem.

@christophstrobl christophstrobl added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 31, 2024
@MrWangGang
Copy link
Author

I find it difficult to provide samples involving databases; it's very troublesome. Perhaps you could simply set up a project.

Define your converter:

@WritingConverter
public class EnumWriteConverter implements Converter<ConverterEnum, String> {
    public EnumWriteConverter() {
    }

    public String convert(ConverterEnum source) {
        return source.getValue();
    }
}

Define the interface:

public interface ConverterEnum {
    String getDescription();

    @JsonValue
    String getValue();
}

Define the enumeration:

public enum EnumCommerceItemDeletedEnum implements ConverterEnum {
    ENUM_COMMERCE_ITEM_DELETED_DELETE("Delete", "DELETE"),
    ENUM_COMMERCE_ITEM_DELETED_UNDELETE("Undelete", "UNDELETE");

    EnumCommerceItemDeletedEnum(String description, String value) {
        this.description = description;
        this.value = value;
    }

    private final String value;
    private final String description;
}

Then create a Spring Data interface:

@Aggregation({
    "{$addFields: { '_idString': { $toString: '$_id' } }}",
    "{$match: { userId: ?0 }}",
    "{$match: { deletedEnum: ?1 }}",
    "{$count: 'totalCount' }"
})
Mono<Long> count(String userId, EnumCommerceItemDeletedEnum deleted);

You can test two scenarios: one based on @Aggregation and one based on a named method. However, I have tested it before, and using repository.find(Example) does not cause such issues. Only when using @query or @Aggregation to execute queries with native SQL, the converter gets called multiple times.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 31, 2024
@MrWangGang
Copy link
Author

@spring-projects-issues

@christophstrobl
Copy link
Member

thanks for the feedback - I see the invocations now. there's multiple things contributing to this. we'll look into those.

@christophstrobl christophstrobl self-assigned this Jun 7, 2024
@christophstrobl christophstrobl removed the status: feedback-provided Feedback has been provided label Jun 7, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 7, 2024
@mp911de mp911de changed the title Converter was called multiple times Converter called multiple times evaluating aggregation operation query methods Jun 12, 2024
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 12, 2024
@mp911de mp911de added this to the 4.2.7 (2023.1.7) milestone Jun 12, 2024
mp911de pushed a commit that referenced this issue Jun 12, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Parsing of string based aggregations lead to multiple invocations of potential converters due to missing reuse of bound parameter value as well as attempts to verify out/merge stages within the pipeline that triggered the stage to be converted into the target document.
The changes in this commit, reduce the number down to 2. One for examining potential expression dependencies and one for the actual conversion and parameter binding.

See #4712
Original pull request: #4717
mp911de added a commit that referenced this issue Jun 12, 2024
Simplify reactive composition. Switch to eager operator evaluation.

See #4712
Original pull request: #4717
mp911de pushed a commit that referenced this issue Jun 12, 2024
Parsing of string based aggregations lead to multiple invocations of potential converters due to missing reuse of bound parameter value as well as attempts to verify out/merge stages within the pipeline that triggered the stage to be converted into the target document.
The changes in this commit, reduce the number down to 2. One for examining potential expression dependencies and one for the actual conversion and parameter binding.

See #4712
Original pull request: #4717
mp911de added a commit that referenced this issue Jun 12, 2024
Simplify reactive composition. Switch to eager operator evaluation.

See #4712
Original pull request: #4717
mp911de pushed a commit that referenced this issue Jun 12, 2024
Parsing of string based aggregations lead to multiple invocations of potential converters due to missing reuse of bound parameter value as well as attempts to verify out/merge stages within the pipeline that triggered the stage to be converted into the target document.
The changes in this commit, reduce the number down to 2. One for examining potential expression dependencies and one for the actual conversion and parameter binding.

See #4712
Original pull request: #4717
mp911de added a commit that referenced this issue Jun 12, 2024
Simplify reactive composition. Switch to eager operator evaluation.

See #4712
Original pull request: #4717
@mp911de mp911de closed this as completed Jun 12, 2024
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
4 participants