-
Notifications
You must be signed in to change notification settings - Fork 224
MobileFuse Adapter: Remove tagid_src and pub_id params #3915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MobileFuse Adapter: Remove tagid_src and pub_id params #3915
Conversation
| private String makeUrl(ExtImpMobilefuse extImp) { | ||
| final String baseUrl = endpointUrl + Objects.toString(extImp.getPublisherId(), "0"); | ||
| return "ext".equals(extImp.getTagidSrc()) ? baseUrl + "&tagid_src=ext" : baseUrl; | ||
| return endpointUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you don't the following part of code as well
final String endpoint = request.getImp().stream()
.map(this::parseImpExt)
.filter(Objects::nonNull)
.findFirst()
.map(this::makeUrl)
.orElse(null);please clean up redundant code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I've removed the redundant code as suggested.
To ensure stability after removing the .map(this::parseImpExt).filter(Objects::nonNull) chain, I added a null check in the modifyImp(Imp imp) method. This prevents a potential NullPointerException in cases where parseImpExt could return null.
Let me know if there's anything else you'd like me to update!
| if (impExt == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this. If there is no impExt then imp will be filtered out for that bidder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I see that in GO they fail on invalid impExt instead of ignoring errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've removed the check as suggested.
|
@tomaszbmf Please, change the logic so that the adapter fails on invalid |
I've updated the code to reintroduce the Previously, the presence of Reintroduces the "Invalid ExtImpMobilefuse value" error when no This change retains the updated endpoint handling logic and does not require any unit test modifications compared to the |
CTMBNara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, the updated code still doesn't comply with GO logic: you need to collect all errors until valid ext
mobileFuseExtension, errs := getFirstMobileFuseExtension(bidRequest)
if errs != nil {
return nil, errs
}
err := jsonutil.Unmarshal(imp.Ext, &bidder_imp_extension)
if err != nil {
errs = append(errs, err)
continue
}
err = jsonutil.Unmarshal(bidder_imp_extension.Bidder, &mobileFuseImpExtension)
if err != nil {
errs = append(errs, err)
continue
}
Also, to simplify the adapter code and bring it closer to the GO logic, I suggest changing it a little as follows:
@Override
public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest request) {
final List<Imp> modifiedImps = new ArrayList<>();
final List<BidderError> errors = new ArrayList<>();
for (Imp imp : request.getImp()) {
try {
if (!isValidImp(imp)) {
continue;
}
final ExtImpMobilefuse extImp = parseImpExt(imp);
modifiedImps.add(modifyImp(imp, extImp));
} catch (PreBidException e) {
errors.add(BidderError.badInput(e.getMessage()));
}
}
if (!errors.isEmpty()) {
return Result.withErrors(errors);
}
final BidRequest modifiedRequest = request.toBuilder().imp(modifiedImps).build();
return Result.withValue(BidderUtil.defaultRequest(modifiedRequest, endpointUrl, mapper));
}
private static boolean isValidImp(Imp imp) {
return imp.getBanner() != null || imp.getVideo() != null || imp.getXNative() != null;
}
private ExtImpMobilefuse parseImpExt(Imp imp) {
try {
return mapper.mapper().convertValue(imp.getExt(), MOBILEFUSE_EXT_TYPE_REFERENCE).getBidder();
} catch (IllegalArgumentException e) {
throw new PreBidException(e.getMessage());
}
}
private Imp modifyImp(Imp imp, ExtImpMobilefuse extImp) {
final ObjectNode skadn = parseSkadn(imp.getExt());
return imp.toBuilder()
.tagid(Objects.toString(extImp.getPlacementId(), "0"))
.ext(skadn != null ? mapper.mapper().createObjectNode().set(SKADN_PROPERTY_NAME, skadn) : null)
.build();
}
private ObjectNode parseSkadn(ObjectNode impExt) {
try {
return mapper.mapper().convertValue(impExt.get(SKADN_PROPERTY_NAME), ObjectNode.class);
} catch (IllegalArgumentException e) {
throw new PreBidException(e.getMessage());
}
}
|
Thanks for the clarification and the code suggestion! 🙏 I’ve now pushed the updated changes incorporating your snippet (with a minor adjustment). Initially, I was aiming to keep the scope minimal and focused strictly on the original change, so I didn’t fully catch the broader alignment you were aiming for with the Golang version. Appreciate your patience, and let me know if anything else needs tweaking! |
| } | ||
|
|
||
| private ObjectNode parseSkadn(ObjectNode impExt) { | ||
| private ObjectNode parseSkadn(ObjectNode impExt) throws PreBidException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for throws
| } | ||
|
|
||
| private ExtImpMobilefuse parseImpExt(Imp imp) { | ||
| private ExtImpMobilefuse parseImpExt(Imp imp) throws PreBidException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorder the methods so that they are placed in the order they are called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied both changes
|
@AntoxaAntoxic @CTMBNara Just following up on the requested changes. Let me know if anything else is needed to move forward with merging. |
This PR removes the
tagid_srcparameter, which was previously used to signal that the publisher is providing their own ID instead of using ours, and thepub_idquery parameter, which was only necessary whentagid_srcwas present. After confirming that publishers do not utilize these parameters, we determined both are unnecessary. Removing them will simplify the setup and reduce confusion for our users.🔧 Type of changes
✨ What's the context?
tagid_srcis effectively not used,pub_idis only necessary whentagid_srcis present🧠 Rationale behind the change
Removing it will help simplify the setup and reduce confusion for our users.
🧪 Test plan
we don't see requests with
tagid_srcquery param🏎 Quality check