spring-projects / spring-data-mongodb

Provides support to increase developer productivity in Java when using MongoDB. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-mongodb/
Apache License 2.0
1.62k stars 1.09k forks source link

Adding `SECONDARY_READS` meta flag to the query does not affect the `readPreference` #4676

Open ryszardmakuch opened 6 months ago

ryszardmakuch commented 6 months ago

Expected behavior: When I add the SECONDARY_READS meta flag using the annotation @Meta(flags = { SECONDARY_READS }) or when .allowSecondaryReads() is invoked on a query, the read preference should be set as secondary or secondaryPreferred.

Actual behaviour: When I add the SECONDARY_READS meta flag using the annotation @Meta(flags = { SECONDARY_READS }) or when .allowSecondaryReads() is invoked on a query, the read preference is instead set as primaryPreferred.

Is this the intended behavior?

Root cause:

The unit test to reproduce the potential bug discovered:

import com.mongodb.ReadPreference
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.springframework.data.mongodb.core.query.Criteria
import org.springframework.data.mongodb.core.query.Query

class TestToReproduceQueryReadPreference {
    // It passes
    @Test
    fun `hasReadPreference returns true if read preference is set`() {
        // given
        val query = Query
            .query(Criteria())
            .withReadPreference(ReadPreference.secondaryPreferred())

        // when
        val hasReadPreference = query.hasReadPreference()

        // then
        assertThat(hasReadPreference).isTrue()
    }

    // It passes
    // Expected : ReadPreference{name=secondaryPreferred, hedgeOptions=null}
    // Actual   : ReadPreference{name=secondaryPreferred, hedgeOptions=null}
    @Test
    fun `query has secondaryPreferred read preference if it is set`() {
        // given
        val query = Query
            .query(Criteria())
            .withReadPreference(ReadPreference.secondaryPreferred())

        // when
        val readPreference = query.readPreference

        // then
        assertThat(readPreference).isEqualTo(ReadPreference.secondaryPreferred())
    }

    // It passes
    @Test
    fun `hasReadPreference returns true if secondary reads are allowed`() {
        // given
        val query = Query
            .query(Criteria())
            .allowSecondaryReads()

        // when
        val hasReadPreference = query.hasReadPreference()

        // then
        assertThat(hasReadPreference).isTrue()
    }

    // It fails
    // Expected : ReadPreference{name=secondaryPreferred, hedgeOptions=null}
    // Actual   : ReadPreference{name=primaryPreferred, hedgeOptions=null}
    @Test
    fun `query has secondaryPreferred read preference if secondary reads are allowed`() {
        // given
        val query = Query
            .query(Criteria())
            .allowSecondaryReads()

        // when
        val readPreference = query.readPreference

        // then
        assertThat(readPreference).isEqualTo(ReadPreference.secondaryPreferred())
    }
}
christophstrobl commented 6 months ago

Thank you for getting in touch. I think we need to work on the documentation side of things here allowSecondaryReads will use the primary if available but allows secondary nodes to be used if not. Please use Query#withReadPreference to set any ReadPreference directly.

ryszardmakuch commented 6 months ago

@christophstrobl, thanks for the explanation. Updating the documentation would be useful.

hadjiski commented 3 months ago

@christophstrobl Wouldn't it make sense to ease the access to this setting by providing a way to define the secondary preference as part of the repository method convention:

@Aggregation(pipeline = {
        { $match: { someDate: { $gte: ?0, $lt: ?1 } } },
        {...},
        {...}
 })
List<SomeEntity> retrieveStatistics

Having to just add the meta flag appears so nice, unfortunately completely not the meaning we expected :) Especially for projects that completely use only high level repository methods, it is cumbersome to have to extract into separate repos and utilizing lower level things just to specify this flag.

So I am not only after more clear documentation, rather after keeping the easy setting

christophstrobl commented 3 months ago

@hadjiski did you have a look at @ReadPreference which is available since 4.2.

hadjiski commented 3 months ago

did you have a look at @ReadPreference which is available since 4.2.

I must have missed this nice annotation, thanks a lot, it works, even a shortcut is available as part of the aggregation one @Aggregation(readPreference = "secondaryPreferred", pipeline = { ... }