51Degrees module: initial commit

[jwrosewell]

Description of change

51Degrees module enriches an incoming OpenRTB request with 51Degrees Device Data.

51Degrees module sets the following fields of the device object: make, model, os, osv, h, w, ppi, pxratio - interested bidder adapters may use these fields as needed. In addition the module sets device.ext.fiftyonedegrees_deviceId to a permanent device ID which can be rapidly looked up in on premise data exposing over 250 properties including the device age, chip set, codec support, and price, operating system and app/browser versions, age, and embedded features.

The module is based on the open source 51Degrees device detection library: https://github.com/51Degrees/device-detection-java.

Integration details are specified in the extra/modules/fiftyone-devicedetection/README.md.

Maintainer contacts:

[email protected]

[AntoxaAntoxic]

Hi @jwrosewell. I see you used scala-like approach for the module, but I must say it's a total overkill for the logic this code should support. Please take a look at other modules and simplify the code.

Since this is an open-source project we prefer to keep everything as similar as possible to be able to maintain this code in future. I hope for your understanding. Thank you.

[justadreamer]

Hi @AntoxaAntoxic thanks for taking a look at the code and for this comment.

My colleague @drasmart (who worked on this module on behalf of 51d) has pointed out that perhaps going with interfaces and implementations and having the vast majority of classes be implementations of a single functional interface (i.e. single method) might seem as an overkill, but he argues that it was done in the name of testability and 100% test coverage that this module achieves.

So before @drasmart jumps into simplifying the code, we'd like to understand what parts or traits specifically you see as overcomplicated and would like to be simplified. Could you please give us a list of these or some examples? So we know exactly what to address and can prioritize our efforts.

Thank you.

[AntoxaAntoxic]

Hi @justadreamer

The main idea of simplifying is having less code, making it more readable and easy to maintain. The less code, from my perspective, leads to less testing code (and 100% coverage is achievable as well). Regarding "readability", it might be my fault, of course, but it took me time to understand that the first hook is only about getting headers and setting them into the module context. And knowing that I think that logic doesn't require all the code. Also these new interfaces bring a lot of wording that the community are not familiar (like evidence, device plan, device patch etc). Thus the next thing "maintainability" will be harder because of that. So I suggest making things more aligned with other existing modules. Our team will definitely appreciate that and will be much easier for us to support you and the module in future.

Regarding the examples, please check any other module. You can start with the richmedia one. It's the smallest as I remember.

On the high-level:

• the first hook: get headers and set headers to the module. I believe all of this can be done in a single hook class
• the 2nd hook: it's more like a converter logic, so basically creating a separate converter service where all the device enrichment logic will be done is fine

You could start with that and then we'll have another review round and come up with the best variant. Thanks!

[justadreamer]

Hi @AntoxaAntoxic! The simplified code has been pushed. The number of files has been significantly reduced and a lot of classes have been collapsed into single one, the javadoc comments have been added to explain essential concepts.
Could you please take a quick look and see if it has reached the reviewable state already or are we at least moving in the right direction. We'd highly appreciate any further guidance you could provide us on the next steps. Thanks.

[AntoxaAntoxic]

General suggestion: let's get rid of functional interfaces. I mean we don't need making each step/method a separate Predicate, Function, Consumer, etc. So please try to get rid of extending the interfaces I mentioned, I believe it will help to achieve the goal.

[AntoxaAntoxic]

I'd say the usage of BiConsumer and EvidenceBuilder is unnecessary

the code coulde be just

@Override
    public Future<InvocationResult<EntrypointPayload>> call(
            EntrypointPayload payload,
            InvocationContext invocationContext)
    {
        return Future.succeededFuture(
                InvocationResultImpl.<EntrypointPayload>builder()
                        .status(InvocationStatus.success)
                        .action(InvocationAction.no_action)
                        .moduleContext(
                                ModuleContext
                                        .builder()
                                         .collectedEvidence(CollectedEvidence.builder().headers(entrypointPayload.headers().entries().build)
                                        .build())
                        .build());
    }

[AntoxaAntoxic]

please just call the methods instead of wrapping them with the functional interfaces

[justadreamer]

Hi @AntoxaAntoxic, the v1.hooks package seems to be cleaned of functional interfaces now with the latest push from @drasmart, could you please take a look and point out other things that need to be worked on? Thanks.

[AntoxaAntoxic]

Hi @justadreamer, thanks for the fix, but my previous comment was related to the core package as well. Almost all the manipulation with the device (getting, patching, planning, detecting, building, merging, all that pipelining stuff etc.) can be done inside the single class/service (maybe 2 or 3, depending), but as I mentioned it doesn't make sense to describe each step as a separate functional interface. All this logic can be done in one place (again, how it's done in other modules).

So I literally suggest merging the logic of the majority of core package functionality in a single DeviceEnrichmentService (the name is not ideal), that will retrieve necessary device info from all the source, change it according to requirements and set the required fields to the payload.

[justadreamer]

@AntoxaAntoxic core package was significantly simplified. Could you please take another look. Thanks

[AntoxaAntoxic]

I've tried to show my vision here (it can be applied to the whole PR, not only the class I've refactored), please check the comments

[AntoxaAntoxic]

public class FiftyOneDeviceDetectionEntrypointHook implements EntrypointHook {

    private static final String CODE = "fiftyone-devicedetection-entrypoint-hook";

    @Override
    public String code() {
        return CODE;
    }

    @Override
    public Future<InvocationResult<EntrypointPayload>> call(
            EntrypointPayload payload,
            InvocationContext invocationContext) {

        final CollectedEvidence evidence = CollectedEvidence.builder()
                .rawHeaders(payload.headers().entries())
                .build();

        return Future.succeededFuture(InvocationResultImpl.<EntrypointPayload>builder()
                        .status(InvocationStatus.success)
                        .action(InvocationAction.no_action)
                        .moduleContext(ModuleContext.builder().collectedEvidence(evidence).build())
                        .build());
    }
}

[justadreamer]

Hi @AntoxaAntoxic, I believe the latest pushed changes address the review comments, could you please take another look when you have a minute. Thanks.

[AntoxaAntoxic]

@justadreamer Did you consider my comment #3156 (comment)?

Due to the latest commit I think partially

[justadreamer]

Hi @AntoxaAntoxic
I think most points of that comment were addressed, please see the detailed response below and let us know if we are missing something and/or specific things that need to be addressed. Thanks.

1. according to our code style the closing figure } should be on the same line with the last method parameter (please use the same style from the example below)

this has been addressed

2. please get rid of passing builder as a parameter (often the object can be built just in place)

the benefit of using a Builder is for testability - we can easily mock it, pass it into the call and then just count the calls made to it

3. I prefer not using PropertyMerge and MergingConfigurator, but instead doing merging in place (again, the example is below)

this has been addressed

4. please use such tools as StringUtils, CollectionUtils and Optional to resolve cases when the object might be null

The problem is that
replacing

        final String model = userAgent.getModel();
        if (model != null && !model.isEmpty()) {
            evidence.put("header.Sec-CH-UA-Model", '"' + toHeaderSafe(model) + '"');
        }

with

        if (StringUtils.isNotEmpty(userAgent.getModel())) {
            headers.put("header.Sec-CH-UA-Model", toHeaderSafe(userAgent.getModel()));
        }

contradicts style guide rule Data retrieval calls of same result

Try not to retrieve same data more than once:
// bad
if (getData() != null) {
final Data resolvedData = resolveData(getData());
...
}

// good
final Data data = getData();
if (data != null) {
final Data resolvedData = resolveData(data);
...
}

^ StringUtils.isNotEmpty adds an additional call to userAgent.getModel(), also it comes from an alien library thus would also contradict the Transitive dependencies code style rule:

Don't use transitive dependencies in project code. If it needed, recommended adding a library as a dependency of Maven in pom.xml

5. get rid of protected methods that used only once in the same class (it's bad practice)

Protected methods are used for the sake of testability - private methods can not be mocked and used in tests.
f.e. see: https://github.com/mockito/mockito/wiki/Mockito-And-Private-Methods

Why Mockito doesn't mock private methods?
We just don't care about private methods because from the standpoint of testing, private methods don't exist.

[AntoxaAntoxic]

Hi @justadreamer, thank you for sharing your feedback on the comments

Regarding the 2nd and the 5th

From my perspective, the testability you mentioned is not hurt by testing only public methods, and the Mockito is enough for that. If the private method is a big enough (and hard to be tested) it can be extracted to a separate class. I prefer not exposing what shouldn't be exposed only for the sake of testability.

Predicting your comment on the paragraph above: the code had been split previously, but was merged into a single hook eventually. Yes, and my comments were not about the collapsing everything. I just wanted to have a more straightforward code that matches the general code style of the repo. For example, the device enrichment still can be extracted and tested separately (but I need to have another look on that).

Also, after the collapsing the method placement contradicts with the style guide: https://github.com/prebid/prebid-server-java/blob/master/docs/developers/code-style.md#method-placement. Also some constants are placed between the methods, but the common rule is having constants at the top of the class, after that other static fields, after that other fields, then the constructor, then the methods according to the guide.

Regarding the 4th
You're right, and the ideal variant will be the following. Sorry for misleading.

        final String model = userAgent.getModel();
        if (StringUtils.isNotEmpty(model)) {
            headers.put("header.Sec-CH-UA-Model", toHeaderSafe(model));
        }

The utility library is widely used in the repo and it's already added in the classpath, so you can freely use it to simplify the code.

---

In the code example from the previous comment I refactored some pieces of code that was bad written, for example the following:

protected boolean isAccountAllowed(AuctionInvocationContext invocationContext) {

        final AccountFilter accountFilter = getModuleConfig().getAccountFilter();
        if (accountFilter == null) {
            return true;
        }
        final List<String> allowList = accountFilter.getAllowList();
        final boolean hasAllowList = allowList != null && !allowList.isEmpty();
        do {
            if (invocationContext == null) {
                break;
            }
            final AuctionContext auctionContext = invocationContext.auctionContext();
            if (auctionContext == null) {
                break;
            }
            final Account account = auctionContext.getAccount();
            if (account == null) {
                break;
            }
            final String accountId = account.getId();
            if (accountId == null || accountId.isEmpty()) {
                break;
            }
            if (hasAllowList) {
                return allowList.contains(accountId);
            }
        } while (false);
        return !hasAllowList;
    }

Please take a another look on that.

---

To summarize: please consider the example of the refactoring I provided earlier, change the order of the methods and fields according to the style guide (you can leave protected methods for the next commit in order test that the suggested refactoring doesn't fail existing tests, but still please order them appropriately). After that I will take another look and suggest further steps.

Thanks!

[justadreamer]

Hi @AntoxaAntoxic

Thanks for the detailed response. We've pushed the changes that should address most of it - please take a look when convenient. Thank you.

Below are comments from Max (@drasmart) who worked on implementing these:

On suggested changes in methods

isAccountAllowed

✔️Applied with changes (that preserve tested behaviour).

updatePayload

if (patchedRequest == null || Objects.equals(patchedRequest, currentRequest))

There is no need to replace reference equality check with value equality one as the new object will not be built if no values are to be appended (to save on both time and garbage).

enrichDevice

final Device enrichedDevice = ObjectUtils.defaultIfNull(bidRequest.getDevice(), Device.builder().build());

Such a construct is sub-optimal performance-wise, as the 2nd argument is always evaluated before the check is run, thus potentially both wasting CPU and producing garbage.

collectEvidence / modifyEvidenceWithDevice

void collectEvidence(
            CollectedEvidence.CollectedEvidenceBuilder evidenceBuilder,
            BidRequest bidRequest)

vs

CollectedEvidence modifyEvidenceWithDevice(CollectedEvidence evidence, Device device)

There are multiple differences:

• The existing signature explicitly conveys the intent of the method -- modifying the writable object.
  • The suggested signature has a much weaker contract, the implementation might return: (1) null, (2) the same or (3) a new object -- which would require diving into the documentation or the actual implementation to figure out how the result should be handled at call-site.
• While it is true that the only currently used part of BidRequest is Device, the underlying library may utilize more info (e.g. domain part from Site, or IP).
  • Embedding this restriction into the contract brings no apparent benefit of readability at call site.
  • What exact evidence is collected from available data IS the implementation detail of 'collection unit' and should not be decapsulated.

Also

return evidence.toBuilder()
        .deviceUA(device.getUa())
        .secureHeaders(makeHeaders(device.getSua()))
        .build();

^ While shorter, this notation differs in functionally.

Currently, collectEvidence is called at 2 points of the hook application -- both before and after returning the Future from the call.

At first point the evidence from payload passed into call is stored inside ModuleContext.
At second point the evidence already in ModuleContext is potentially appended with the latest data from t passed into InvocationResult.payloadUpdate.

Thus, it is possible for the suggested shorter form to overwrite previously-saved (at call time) data with null-s (at payloadUpdate time) -- which is not benefitial.

addEvidenceToContext

✔️ Applied with changes (signature preserved).

• Consumer<CollectedEvidence.CollectedEvidenceBuilder> evidenceInjector is a part of the contract because this 'unit' IS responsible for both deciding how to BUILD a writable object and how to INJECT the filled-in result (made out of said writable object) into new ModuleContext.
• This separates out both WHAT/HOW the created writable object is POPULATED and WHERE the data for doing it comes FROM.

Combined the points above significantly reduce the respobsibilities scope, making the code less prone to change, thus increasing maintainability.

makeHeaders

✔️ Collection/Map/String utils applied.

brandListToString

return versions.stream()
        .filter(version -> Objects.nonNull(version.getBrand()))
        .map(version -> toHeaderSafe(version.getBrand()) + ";v=" + String.join(".", version.getVersion()))
        .collect(Collectors.joining(", "));

I do agree this is more readable, but

in the context of style guide recommending the way of reducing the number of plain getters called,

I hope you would agree for it to be in the spirit of style guide to follow the best practice of using StringBuilder to avoid the unnecessary allocations of intermediate String objects (at both levels of arrays).

[AntoxaAntoxic]

please move to a separate enum class, and rename it to OrtbDeviceType

[AntoxaAntoxic]

please move to a separate class

[AntoxaAntoxic]

Regarding unit tests style:

• usually we have one test class per the class being tested
• the class that under testing is called a target and it's usually created once per the test class (it might be an exceptions of course, but I believe not in this case)
• it's not necessary to mock if it's possible to create a given object
• the method order rules are the same for the tests as well as for the other part of the code
• the name convention for the test cases is the <testing public method name>Should<expected result>When<pre-condition> (please check my example from the previous comment)

And please use other unit tests in the repo as inspiration.

[AntoxaAntoxic]

the premade builder is always null, so there is no sense to keep it

[AntoxaAntoxic]

    Pipeline pipeline(ModuleConfig moduleConfig) throws Exception {
        return PipelineBuilder.create(moduleConfig).build();
    }

[AntoxaAntoxic]

is it correct to return? or is it a mistake?

[AntoxaAntoxic]

please make it an utility class

public class PipelineBuilder {
    private static final Collection<String> PROPERTIES_USED = List.of(
            "devicetype",
            "hardwarevendor",
            "hardwaremodel",
            "hardwarename",
            "platformname",
            "platformversion",
            "screenpixelsheight",
            "screenpixelswidth",
            "screeninchesheight",
            "pixelratio",

            "BrowserName",
            "BrowserVersion",
            "IsCrawler",

            "BrowserVendor",
            "PlatformVendor",
            "Javascript",
            "GeoLocation",
            "HardwareModelVariants");

    private PipelineBuilder() {

    }

    public static PipelineBuilderBase<?> create(ModuleConfig moduleConfig) throws Exception {
        final DataFile dataFile = moduleConfig.getDataFile();
        final DeviceDetectionOnPremisePipelineBuilder builder = makeRawBuilder(dataFile);
        applyUpdateOptions(builder, dataFile.getUpdate());
        applyPerformanceOptions(builder, moduleConfig.getPerformance());
        PROPERTIES_USED.forEach(builder::setProperty);
        return builder;
    }

    private static DeviceDetectionOnPremisePipelineBuilder makeRawBuilder(DataFile dataFile) throws Exception {
        final Boolean shouldMakeDataCopy = dataFile.getMakeTempCopy();
        return new DeviceDetectionPipelineBuilder()
                .useOnPremise(dataFile.getPath(), BooleanUtils.isTrue(shouldMakeDataCopy));
    }

    private static void applyUpdateOptions(DeviceDetectionOnPremisePipelineBuilder pipelineBuilder,
                                           DataFileUpdate updateConfig) {
        pipelineBuilder.setDataUpdateService(new DataUpdateServiceDefault());

        final Boolean auto = updateConfig.getAuto();
        if (auto != null) {
            pipelineBuilder.setAutoUpdate(auto);
        }

        final Boolean onStartup = updateConfig.getOnStartup();
        if (onStartup != null) {
            pipelineBuilder.setDataUpdateOnStartup(onStartup);
        }

        final String url = updateConfig.getUrl();
        if (StringUtils.isNotBlank(url)) {
            pipelineBuilder.setDataUpdateUrl(url);
        }

        final String licenseKey = updateConfig.getLicenseKey();
        if (StringUtils.isNotBlank(licenseKey)) {
            pipelineBuilder.setDataUpdateLicenseKey(licenseKey);
        }

        final Boolean watchFileSystem = updateConfig.getWatchFileSystem();
        if (watchFileSystem != null) {
            pipelineBuilder.setDataFileSystemWatcher(watchFileSystem);
        }

        final Integer pollingInterval = updateConfig.getPollingInterval();
        if (pollingInterval != null) {
            pipelineBuilder.setUpdatePollingInterval(pollingInterval);
        }
    }

    private static void applyPerformanceOptions(DeviceDetectionOnPremisePipelineBuilder pipelineBuilder,
                                                PerformanceConfig performanceConfig) {
        final String profile = performanceConfig.getProfile();
        if (StringUtils.isNotBlank(profile)) {
            for (Constants.PerformanceProfiles nextProfile : Constants.PerformanceProfiles.values()) {
                if (StringUtils.equalsAnyIgnoreCase(nextProfile.name(), profile)) {
                    pipelineBuilder.setPerformanceProfile(nextProfile);
                    // todo: return or break the cycle?
                    return;
                }
            }
        }

        final Integer concurrency = performanceConfig.getConcurrency();
        if (concurrency != null) {
            pipelineBuilder.setConcurrency(concurrency);
        }

        final Integer difference = performanceConfig.getDifference();
        if (difference != null) {
            pipelineBuilder.setDifference(difference);
        }

        final Boolean allowUnmatched = performanceConfig.getAllowUnmatched();
        if (allowUnmatched != null) {
            pipelineBuilder.setAllowUnmatched(allowUnmatched);
        }

        final Integer drift = performanceConfig.getDrift();
        if (drift != null) {
            pipelineBuilder.setDrift(drift);
        }
    }
}

[CTMBNara]

Please, consider about proposed approach

[justadreamer]

Hi @AntoxaAntoxic

Thanks for the review as always. @drasmart has addressed the above, here are his comments:

usually we have one test class per the class being tested

✔️ Merged.

the class that under testing is called a target and it's usually created once per the test class (it might be an exceptions of course, but I believe not in this case)

✔️ Renamed.

it's not necessary to mock if it's possible to create a given object

Assuming this was about using mock to setup implementations of [AuctionRequestPayload] / [AuctionInvocationContext].

✔️ Replaced with construction of [AuctionRequestPayloadImpl] / [AuctionInvocationContextImpl].

the method order rules are the same for the tests as well as for the other part of the code

✔️ Updated.

the name convention for the test cases is the <testing public method name>Should<expected result>When<pre-condition>

✔️ Renamed.

the premade builder is always null, so there is no sense to keep it

As previously explained, the changes to DeviceDetectionOnPremisePipelineBuilder can not be observed on the built Pipeline.

Thus, a way to inject the mock to check for method calls IS necessary for testing.

please make it an utility class

• Made as much methods static as possible.
• Moved premadeBuilder parameter setter out of constructor.
• Renamed to PipelineBuilderBuilder to reflect following the builder pattern to get PipelineBuilderBase<?> instance.

is it correct to return? or is it a mistake?

✔️ Extracted into private method.

[AntoxaAntoxic]

just mock the enricher

[AntoxaAntoxic]

the cases that check the headers and so on should be in the SecureHeadersRetrieverTest, not here

here only the hook logic should be tested

[justadreamer]

these have been addressed with the latest commit (below), thanks

[AntoxaAntoxic]

as I can judge premadeBuilder is used only for testing purpose, this is the bad practice

please compare the builder is expected OR I suggest returning a Pipeline object on calling the build, in that way you can check whether the Pipeline has the state you expected after building

[justadreamer]

I've got the following explanation from @drasmart why this is implemented this way (if I understood and paraphrased it correctly):

• [PipelineDefault] class is inaccessible.
  • The state of this class is not a part of the Pipeline interface contract.
  • The PipelineBuilder classes (of library, not module) are required to return a Pipeline that behaves in a certain way, and it is the library's internal concern, not modules - so the state is tested separately in the library.
• PipelineBuilderBuilder is the boundary class (between the module and library code, or one that is wrapping library code).
  • It's responsibility is invoking side-effectful foreign (library's) APIs.
  • Counting invocations of the APIs of the passed mock object is done to test that the behavior actually supports the contract.

Please let us know if this is an ok approach to test it this way, or should we seek some other way to implement this boundary/wrapper. Thanks.

[AntoxaAntoxic]

The primary option:

PipelineDefault is using internally and it's fine, but the PipelineBuilder builds that object, and further in the code the Pipeline interface represents it. And that's totally fine. From my perspective you don't need to test the building itself, you can check the output whether the result Pipeline behaves in the way you expect from it.

I don't understand why you call "checking whether the Pipeline is built correctly" an internal concern if you build it inside the module and use the result of this building further in the code.

What is you're trying to cover is only the building steps, that only proves that the builder calls setters, but the output result and it's validness is not clear.

The alternative (I don't like but I don't understand why the primary one is not sufficient)

public class PipelineBuilder {
    
    private final ModuleConfig moduleConfig;

    public PipelineBuilder(ModuleConfig moduleConfig) {
        this.moduleConfig = moduleConfig;
    }

    public Pipeline build(DeviceDetectionPipelineBuilder premadeBuilder) throws Exception {
        final DataFile dataFile = moduleConfig.getDataFile();
        
        final Boolean shouldMakeDataCopy = dataFile.getMakeTempCopy();
        final DeviceDetectionOnPremisePipelineBuilder builder = premadeBuilder.useOnPremise(
                dataFile.getPath(), 
                BooleanUtils.isTrue(shouldMakeDataCopy));
        
        applyUpdateOptions(builder, dataFile.getUpdate());
        applyPerformanceOptions(builder, moduleConfig.getPerformance());
        PROPERTIES_USED.forEach(builder::setProperty);
        return builder.build();
    }
    
    ...

}

and the hook

    public FiftyOneDeviceDetectionRawAuctionRequestHook(ModuleConfig moduleConfig) throws Exception {
        this.moduleConfig = Objects.requireNonNull(moduleConfig);
        final Pipeline pipeline = new PipelineBuilder(moduleConfig).build(new DeviceDetectionPipelineBuilder());
        this.deviceEnricher = new DeviceEnricher(pipeline);
    }

[AntoxaAntoxic]

I believe throwing an exception is not necessary in these tests

[justadreamer]

✔️ Cleaned up

[AntoxaAntoxic]

please, consider using @Value (and @Builder) for the classes in the config because usually config is created once and stay immutable with only reading access (at your current usage looks like that)

[justadreamer]

we see that a couple other modules use @Data annotations in the config:

• https://github.com/prebid/prebid-server-java/blob/master/extra/modules/confiant-ad-quality/src/main/java/org/prebid/server/hooks/modules/com/confiant/adquality/config/ConfiantAdQualityModuleConfiguration.java#L68-L78
• https://github.com/prebid/prebid-server-java/blob/master/extra/modules/confiant-ad-quality/src/main/java/org/prebid/server/hooks/modules/com/confiant/adquality/model/RedisRetryConfig.java#L5-L6
• https://github.com/prebid/prebid-server-java/blob/master/extra/modules/confiant-ad-quality/src/main/java/org/prebid/server/hooks/modules/com/confiant/adquality/model/RedisConnectionConfig.java#L5-L6

I believe @drasmart tried to use @Builder, but for some reason Spring did not produce the necessary code... Perhaps a skill issue, but we could not find a good example how to do that. If you could please provide an example - we are willing to rewrite. Thanks.

[AntoxaAntoxic]

yeah, my bad, since it's the ConfigurationProperties that should be @Data annotated

[AntoxaAntoxic]

The primary option:

PipelineDefault is using internally and it's fine, but the PipelineBuilder builds that object, and further in the code the Pipeline interface represents it. And that's totally fine. From my perspective you don't need to test the building itself, you can check the output whether the result Pipeline behaves in the way you expect from it.

I don't understand why you call "checking whether the Pipeline is built correctly" an internal concern if you build it inside the module and use the result of this building further in the code.

What is you're trying to cover is only the building steps, that only proves that the builder calls setters, but the output result and it's validness is not clear.

The alternative (I don't like but I don't understand why the primary one is not sufficient)

public class PipelineBuilder {
    
    private final ModuleConfig moduleConfig;

    public PipelineBuilder(ModuleConfig moduleConfig) {
        this.moduleConfig = moduleConfig;
    }

    public Pipeline build(DeviceDetectionPipelineBuilder premadeBuilder) throws Exception {
        final DataFile dataFile = moduleConfig.getDataFile();
        
        final Boolean shouldMakeDataCopy = dataFile.getMakeTempCopy();
        final DeviceDetectionOnPremisePipelineBuilder builder = premadeBuilder.useOnPremise(
                dataFile.getPath(), 
                BooleanUtils.isTrue(shouldMakeDataCopy));
        
        applyUpdateOptions(builder, dataFile.getUpdate());
        applyPerformanceOptions(builder, moduleConfig.getPerformance());
        PROPERTIES_USED.forEach(builder::setProperty);
        return builder.build();
    }
    
    ...

}

and the hook

    public FiftyOneDeviceDetectionRawAuctionRequestHook(ModuleConfig moduleConfig) throws Exception {
        this.moduleConfig = Objects.requireNonNull(moduleConfig);
        final Pipeline pipeline = new PipelineBuilder(moduleConfig).build(new DeviceDetectionPipelineBuilder());
        this.deviceEnricher = new DeviceEnricher(pipeline);
    }

[justadreamer]

Hi @AntoxaAntoxic Thanks for the latest review notes above - I believe the changes were applied in the latest commit by @drasmart.

[AntoxaAntoxic]

Overall, looks good. Thank you, guys!

A few small comments to fix have been left.

[AntoxaAntoxic]

private

[justadreamer]

✔️ Applied

[AntoxaAntoxic]

private

[justadreamer]

✔️ Applied

[AntoxaAntoxic]

the exception is handled and returned, but never used further, do you really need it?

[justadreamer]

This is a great question. Maybe the design decision here was not the best. Here is Max's thinking on the question:

❓> Do we actually need to handle the exception this way?

What is the exact contract on exception handling within payloadUpdate ?

• Should the module wrap the pipeline exception into RuntimeException and let it bubble up and be caught by [GroupResult.applyPayloadUpdate] within module's host (server app) ?
  • Will the rest of the auction continue as though the module didn't interfere at all?
• Should the payloadUpdate handler amend failures or tags to the already-returned InvocationResult ?
• Is the module responsible for logging errors inside payloadUpdate?
  • Should they be aggregated by some criteria (e.g. publisher account) ?

The exception was saved in the EnrichmentResult instance to be handled later as this didn't seem to be critical before. Maybe we are just overcomplicating stuff and it should be just rethrown, or absorbed completely...

Please share your thoughts, thank you.

[AntoxaAntoxic]

My initial point here is based on the fact that the exception you catch is never used. So from the code perspective we usually get rid of the stuff that are never used (I believe that's obvious). So there are two ways: 1) catch and do nothing and 2) catch and do something (here I'm mainly describing the exceptions that disrupts the module in general, so the module logic can not be applied at all). It's never worth (in both cases) to throw the exception from the module without handling it, since it's worth not to update the payload at all and return a debug/error message and no_update action status instead in the exception case.

Do nothing is basically what happens in your case, looks like you ignore the exception at all, if it's the desired behavior I can only suggest remove the processedException field from the EnrichmentResult and, as an idea, create an EMPTY EnrichmentResult to return in this case.

Do something is really depends on your desire. For example, you can log the exception for the observability purpose OR you can add a debug or error message to the invocation result and notify the client on the response.

[justadreamer]

Thanks for the provided explanation.

✔️ Removed this code.
➡️ Added throws Exception to DeviceEnricher.populateDeviceInfo and RawAuctionRequestHook.enrichDevice method signatures.
➡️ Moved catch clause from try-with-resource in DeviceEnricher.populateDeviceInfo to RawAuctionRequestHook.updatePayload.

[AntoxaAntoxic]

does this comparison even matters?
I believe it could be just as the following

return patchedRequest == null ? existingPayload : AuctionRequestPayloadImpl.of(patchedRequest);

[justadreamer]

✔️ Applied

[justadreamer]

Hi @AntoxaAntoxic I believe the above have been addressed in the latest push. Is there anything else you would like us to address in order to approve the PR? Thanks.

[CTMBNara]

Please, consider about proposed approach

[CTMBNara]

What about this approach? :p

    private static void appendVersionList(StringBuilder stringBuilder, List<String> versions) {
        if (CollectionUtils.isEmpty(versions)) {
            return;
        }

        stringBuilder.append('.');
        stringBuilder.append(versions.getFirst());
        for (int i = 1; i < versions.size(); i++) {
            stringBuilder.append(versions.get(i));
        }
    }

[justadreamer]

⚠️ That would invert the logic:

• from "before each segment except first"
  • ["111", "0", "5563", "146"] -> "111.0.5563.146"
• to "before first segment only"
  • ["111", "0", "5563", "146"] -> ".11105563146"

[CTMBNara]

Ah, yea, my bad... Tnx for pointing that out!

    private static void appendVersionList(StringBuilder stringBuilder, List<String> versions) {
        if (CollectionUtils.isEmpty(versions)) {
            return;
        }

        stringBuilder.append(versions.getFirst());
        for (int i = 1; i < versions.size(); i++) {
            stringBuilder.append('.');
            stringBuilder.append(versions.get(i));
        }
    }

[justadreamer]

✔️ Applied, thanks

[justadreamer]

Hi @CTMBNara thanks for the review comments, the requested changes have been implemented. Please let us know if more changes are needed.

[SerhiiNahornyi]

Last round.

[SerhiiNahornyi]

Nitpick: Please remove Javadocs. We are trying to remove them everywhere since 99% of the code and argument names are self-explanatory.

[justadreamer]

✔️ Applied

[SerhiiNahornyi]

Same for here, and also check for other occurrences.

[justadreamer]

✔️ Applied

[SerhiiNahornyi]

Redundant

[justadreamer]

✔️ Applied

[SerhiiNahornyi]

LGTM 👍, will be part of the next PBS release.