Examples in reactive-boot-mongo are not working properly

[ghost]

Hi,

there are a few issues with the implementations in the reactive chapter. I was reading and taking a look at reactive-boot-mongo project and there are several issue with save and update methods.

Mono<Person> personMono = serverRequest.bodyToMono(Person.class).doOnNext(personService::save);

is translated to

Mono<Person> personMono = serverRequest.bodyToMono(Person.class)
.doOnNext(new Consumer<Person>() {
            @Override
            public void accept(Person person1) {
                Mono<Person> save = personService.save(person1);
            }
        });

if I convert the personService::save to an anonymous class. The Mono returned by personService.save is not returned so it's never called.

This can be demonstrated if shouldCreateAPerson is updated like this

 void shouldCreateAPerson() {
        Person person = new Person();
        person.setId(\"catherine");
        person.setUsername("catherine.cawood");
        person.setFirstName("Catherine");
        person.setLastName("Cawood");
        person.setPassword("ccwoo");
        person.setHiringDate(DateProcessor.toDate("1986-05-27 00:38"));

        webTestClient.post().uri("/").body(Mono.just(person), Person.class).exchange().expectStatus().isCreated();
        webTestClient.get().uri(\"/catherine\").accept(MediaType.TEXT_EVENT_STREAM)
                .exchange()
                .expectStatus().isOk()
                .expectHeader().contentType(MediaType.APPLICATION_JSON)
                .expectBody(Person.class).consumeWith(responseEntity -> {
            Person catherine = responseEntity.getResponseBody();
            assertAll("catherine", () ->
            {
                assertNotNull(catherine);
                assertAll("catherine",
                        () -> assertEquals("Catherine", catherine.getFirstName()),
                        () -> assertEquals("Cawood", catherine.getLastName()));
            });

        });
    }

The test will fail with Status expected:<200 OK> but was:<404 NOT_FOUND> when fetching the person identified by "catherine".

I encountered a similar issue with the update method from PersonServiceImpl.

I can open a pull request for the mentioned issues, if that's ok!

[iuliana]

Hello Ovidiu,

There is no correction needed. The examples work as intended.

You are in a reactive environment, the two webTestClient.post() and webTestClient.get() pipelines are executed on different threads, ofcourse the test fails sometimes. Because when executing the webTestClient.get() pipeline you have no way to know the whole webTestClient.post() pipeline was executed and the result saved into the database.

If you want to test the result in your post(), the test you need looks more like this (basically add your assertions to the pipeline):

    @Test
    void shouldCreateAPerson(){
        Person person = new Person();
        person.setId(UUID.randomUUID().toString());
        person.setUsername("catherine.cawood");
        person.setFirstName("Catherine");
        person.setLastName("Cawood");
        person.setPassword("ccwoo");
        person.setHiringDate(DateProcessor.toDate("1986-05-27 00:38"));

        webTestClient.post().uri("/").body(Mono.just(person), Person.class).exchange().expectStatus().isCreated()
                .expectBody(Person.class).consumeWith(p -> {
            assertNotNull(person);
            assertAll("person",
                    () -> assertNotNull(person.getId()),
                    () -> assertEquals("Catherine", person.getFirstName()),
                    () -> assertEquals("Cawood", person.getLastName()));
                }
        );
    }

Also, you are saying that the result of doOnNext(..) is lost?? Then how come if you put a breakpoint in the PersonHandler.save() method, where the ServerResponse is created, you can see an id ? Populated ID Image

Look at the code there, the accept method in the consumer doesn't return anything, but then how come you have the result of the whole execution pipeline assigned to a Mono<T>? Why does that code compile then ?? If you want to how and why, you can look at the code in all classes in the Mono<T> hierarchy.

Cheers, Iuliana

[ghost]

Hi again @iuliana ,

the id comes from serverRequest.bodyToMono(Person.class) not from.doOnNext(personService::save). doOnNext is a consumer of the element emitted by the bodyToMono method.

I may be wrong (as I am not an expert) but I think it's worth checking out if the entity is actually persisted in the database.

If I expand serverRequest.bodyToMono(Person.class).doOnNext(personService::save) to

Mono<Person> personMono = serverRequest.bodyToMono(Person.class).doOnNext(new Consumer<Person>() {
            @Override
            public void accept(Person person) {
                LOGGER.info("Accept");
                personService.save(person).doOnNext(dbPerson -> LOGGER.info("person persisted"));
            }
        });

logs will look like this, LOGGER.info("person persisted") will never be printed

If I manually subscribe to that Publisher, personService.save(person).doOnNext(dbPerson -> LOGGER.info("person persisted")).subscribe(); that logger is also printed

[iuliana]

Hello Ovidiu,

Your insistence has made me curious. I've reopened this issue and I will look more into it.

Cheers!

[iuliana]

Hello Ovidiu,

You were right. No idea why I went for the implementation using doOnNext(..). The reason why that does not work is because there is no subscriber. (Dooh!)

Yeap, the id was coming from my test because I set it there... Why? I have no idea, the book was written two years ago, I can only blame sleep deprivation as always(or maybe I was a shittier developer at the time 🍡 ). I did not even noticed it until you pointed it out. The id was supposed to be generated by persisting the object to the database. That is why I was so sure everything went well. :|

You are saying you are not an expert. Turns out I am not one either. :)

So the fix for scenario is to add the save operation to the pipeline in PersonHandler

    public Mono<ServerResponse> save(ServerRequest serverRequest) {
        return serverRequest.bodyToMono(Person.class)
                .flatMap(person -> personService.save(person))
                .flatMap(person -> ServerResponse.created(
                        URI.create("/persons/" + person.getId())
                ).contentType(MediaType.APPLICATION_JSON).bodyValue(person))
                .switchIfEmpty(ServerResponse.notFound().build());
    }

And to correct the test.

@Test
    void shouldCreateAPerson(){
        Person person = new Person();
        person.setUsername("catherine.cawood");
        person.setFirstName("Catherine");
        person.setLastName("Cawood");
        person.setPassword("ccwoo");
        person.setHiringDate(DateProcessor.toDate("1986-05-27 00:38"));

        webTestClient.post().uri("/").body(Mono.just(person), Person.class).exchange().expectStatus().isCreated()
                .expectBody(Person.class).consumeWith(responseEntity -> {
            Person p = responseEntity.getResponseBody();
            assertNotNull(p);
            assertAll("person",
                    () -> assertNotNull(p.getId()),
                    () -> assertEquals("Catherine", p.getFirstName()),
                    () -> assertEquals("Cawood", p.getLastName()));
                }
        );
    }

Thank you for this observation, this is a very nice catch. And thank you for being persistent about it until I took the time to seriously look into it. I will add the changes to the repo, after I make a scroll through the whole application to make sure I have no other stupid code like this. Also, I will add you to the contributors list, unless you have anything against it. ;)

Cheers, Iuliana Cosmina

[ghost]

Hi @iuliana ,

you are welcome. Sure, no problem, you can add me to the contributors list.

Nice book by the way. I enjoyed reading it :)

Cheers, Ovidiu

[iuliana]

Thank you @ovidiupopa91 , I am happy you think so. :) Good luck and keep in touch!