spring-data-mongodb icon indicating copy to clipboard operation
spring-data-mongodb copied to clipboard

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

Open ryszardmakuch opened this issue 1 year ago • 5 comments

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?

  • If it is, could you please explain why?
  • If not, would you mind if I created a separate PR to correct it, as per the expected behavior outlined above?

Root cause:

  • https://github.com/spring-projects/spring-data-mongodb/blob/d294d50407b435adbe2f9cffbbbdcd548801382f/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/query/Query.java#L236C1-L249C3

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())
    }
}

ryszardmakuch avatar Mar 29 '24 10:03 ryszardmakuch

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.

christophstrobl avatar Apr 04 '24 13:04 christophstrobl

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

ryszardmakuch avatar Apr 04 '24 13:04 ryszardmakuch

@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

hadjiski avatar Jul 04 '24 16:07 hadjiski

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

christophstrobl avatar Jul 05 '24 05:07 christophstrobl

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 = { ... }

hadjiski avatar Jul 05 '24 12:07 hadjiski