Skip to content

NextMillennium: Adapter and server version #3814

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

Merged
merged 13 commits into from
May 16, 2025

Conversation

przemkaczmarek
Copy link
Collaborator

🔧 Type of changes

  • bid adapter update

✨ What's the context?

#3744

Comment on lines 108 to 113
final ExtRequestPrebid updatedExt = extRequestPrebid.toBuilder()
.nextMillennium(ExtRequestNextMillennium.of(
nmmFlags,
NM_ADAPTER_VERSION,
versionProvider.getNameVersionRecord()))
.build();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go they don't update ext or ext.prebid, they create new objects

Comment on lines 115 to 118
return bidRequest.toBuilder()
.imp(bidRequest.getImp())
.ext(ExtRequest.of(updatedExt))
.build();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imp[0].ext update is missing

/**
* Defines the contract for bidrequest.ext.prebid.nextMillennium
*/
ExtRequestNextMillennium nextMillennium;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deserialization is incorrect here

List<String> nmmFlags;

@JsonProperty("nm_version")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its not redutat because i got from server:
image

and how about "still incorrect, it should be nmmFlags" i have got n m m F l a g s . where is a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the annotation is redundant

i have got n m m F l a g s . where is a problem? Sorry, I didn't get. According to the Go the field should be serialized into the nnmFlags instead of nnm_flags

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nnm or nmm ? I have got nmm in Go and Java
image

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point was about the case, it should be nmmFlags instead of the nmm_flags in the bidder request

String nmVersion;

@JsonProperty("server_version")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant

final ObjectNode nextMillenniumNode = mapper.mapper().valueToTree(
ExtRequestNextMillennium.of(nmmFlags, NM_ADAPTER_VERSION, versionProvider.getNameVersionRecord()));

extRequest.addProperty("next_millennium", nextMillenniumNode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect property name, should be nextMillennium

Comment on lines 115 to 116
final ObjectNode updatedImpExt = mapper.mapper().createObjectNode();
updatedImpExt.set("bidder", mapper.mapper().valueToTree(ext));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this? I don't see any bidder field in the bidder request

Copy link
Collaborator Author

@przemkaczmarek przemkaczmarek Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in L11-15 there is bidder field:

"ext": {
        "bidder": {
          "placement_id": "placement_id"
        }
      }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Go doesn't have it

.build();

return bidRequest.toBuilder()
.imp(bidRequest.getImp())
.ext(ExtRequest.of(updatedExt))
.imp(List.of(updatedImp))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should update only a first imp, but not replace the whole list of imps with a single one

@JsonProperty("nmmFlags")
List<String> nmmFlags;

@JsonProperty("nmVersion")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove annotation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep it because when i remove it test failed.
image

from server i recived fields with _
fe. i recived "nmm_flags"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means the test is incorrect

this is the mapping the Go uses

type nmExtNMM struct {
	NmmFlags       []string `json:"nmmFlags,omitempty"`
	ServerVersion  string   `json:"server_version,omitempty"`
	AdapterVersion string   `json:"nm_version,omitempty"`
}

@JsonProperty("nmVersion")
String nmVersion;

@JsonProperty("serverVersion")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove annotation

@@ -112,7 +112,11 @@ private BidRequest updateBidRequest(BidRequest bidRequest, ExtImpNextMillennium

final Imp firstImp = bidRequest.getImp().getFirst();
final ObjectNode updatedImpExt = mapper.mapper().createObjectNode();
updatedImpExt.set("bidder", mapper.mapper().valueToTree(ext));
updatedImpExt.set("nextMillennium", nextMillenniumNode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nextMillenniumNode should have only nmmFlags if it's an imp.ext

@@ -112,7 +112,7 @@ private BidRequest updateBidRequest(BidRequest bidRequest, ExtImpNextMillennium

final Imp firstImp = bidRequest.getImp().getFirst();
final ObjectNode updatedImpExt = mapper.mapper().createObjectNode();
updatedImpExt.set("nextMillennium", nextMillenniumNode);
updatedImpExt.set("nextMillennium", nextMillenniumNode.get("nmmFlags"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is incorrect, please observe the Go code one more time

@@ -92,7 +100,7 @@ private ExtImpNextMillennium convertExt(ObjectNode impExt) {
}
}

private BidRequest updateBidRequest(BidRequest bidRequest, ExtImpNextMillennium ext) {
private BidRequest updateBidRequest1(BidRequest bidRequest, ExtImpNextMillennium ext) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

AntoxaAntoxic
AntoxaAntoxic previously approved these changes Mar 19, 2025
@osulzhenko osulzhenko requested a review from CTMBNara March 27, 2025 10:44
…3744

# Conflicts:
#	src/main/java/org/prebid/server/bidder/nextmillennium/NextMillenniumBidder.java
@CTMBNara CTMBNara merged commit 06893a5 into master May 16, 2025
8 checks passed
@CTMBNara CTMBNara deleted the NextMillennium-Adapter-and-server-version-#3744 branch May 16, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants