Merge site object for SmartadserverBidder

[muuki88]

The Smartadserver adapter overrides the entire site object, which removes all openrtb extensions like domain, page or any other information that has been set.

[muuki88]

I'm coming from Scala with a focus on functional styles. Is this kind of programming style okay for this project?

[SerhiiNahornyi]

Usually we prefer to use methods like modify[DesiredObject], like this

private Site modifySite(Site site, Integer networkId) {
        final Site.SiteBuilder siteBuilder = site != null ? site.toBuilder() : Site.builder();
        final Publisher sitePublisher = site != null ? site.getPublisher() : null;

        return siteBuilder.publisher(modifyPublisher(sitePublisher, networkId)).build();
    }

    private Publisher modifyPublisher(Publisher publisher, Integer networkId) {
        final Publisher.PublisherBuilder publisherBuilder = publisher != null
                ? publisher.toBuilder()
                : Publisher.builder();

        return publisherBuilder.id(String.valueOf(networkId)).build();
    }

Then your bidRequest can be created like

final BidRequest updatedRequest = request.toBuilder()
                        .imp(Collections.singletonList(imp))
                        .site(modifySite(request.getSite(), extImpSmartadserver.getNetworkId()))
                        .build();

[muuki88]

I refactored according to your suggestions 😃

Out of curiosity. Was not using Optional an explicit decisions and if why so? Performance, too much refactoring? I was thinking about opening an issue to discuss this as I'm stumbling over NPEs quite a lot 😂 (not used to null anymore)

[SerhiiNahornyi]

We usually do like this

final Site expectedSite = Site.builder()
                .domain("www.foo.com")
                .publisher(Publisher.builder().domain("foo.com").id("4").build())
                .build();
        assertThat(result.getErrors()).isEmpty();
        assertThat(result.getValue())
                .extracting(HttpRequest::getPayload)
                .extracting(BidRequest::getSite)
                .containsExactly(expectedSite);

[muuki88]

That looks nicer 😄

[SerhiiNahornyi]

Usually we prefer to use methods like modify[DesiredObject], like this

private Site modifySite(Site site, Integer networkId) {
        final Site.SiteBuilder siteBuilder = site != null ? site.toBuilder() : Site.builder();
        final Publisher sitePublisher = site != null ? site.getPublisher() : null;

        return siteBuilder.publisher(modifyPublisher(sitePublisher, networkId)).build();
    }

    private Publisher modifyPublisher(Publisher publisher, Integer networkId) {
        final Publisher.PublisherBuilder publisherBuilder = publisher != null
                ? publisher.toBuilder()
                : Publisher.builder();

        return publisherBuilder.id(String.valueOf(networkId)).build();
    }

Then your bidRequest can be created like

final BidRequest updatedRequest = request.toBuilder()
                        .imp(Collections.singletonList(imp))
                        .site(modifySite(request.getSite(), extImpSmartadserver.getNetworkId()))
                        .build();

[SerhiiNahornyi]

Actually this test also checks that we update Site object, so it will be great to rename it like
makeHttpRequestsShouldUpdateSiteObjectIfPresent or another one

[muuki88]

Thanks a lot for the detailed code review ❤️ I implemented all the requested changes.

[SerhiiNahornyi]

One more small change, pls, make methods modifySite and modifyPublisher to be static

[rpanchyk]

@muuki88 agree?

[muuki88]

Will do! I must have missed that comment.