the user id module should pull the ID from cache automatically if using the core id module to save it (as opposed to doing it manually); extendId should not be required unless you're trying to do something extra with the cached value on every page, which in this PR, it doesn't look like you're doing. so it seems like there's an issue with the page setup or something else is going on to cause the issue you're seeing.

the user id module should pull the ID from cache automatically if using the core id module to save it (as opposed to doing it manually); extendId should not be required unless you're trying to do something extra with the cached value on every page, which in this PR, it doesn't look like you're doing. so it seems like there's an issue with the page setup or something else is going on to cause the issue you're seeing.

Thanks for your response, @smenzer. Unfortunately this does not look like a setup issue. I am able to replicate this locally as well. Here's how the flow works:

1. DeepIntent script is responsible for setting the token in local storage. This is done in the format: key : {"id": "123123123"}.
2. In the prebid code, "populateSubmoduleId" method is called in the init flow. In the intial flow idSystem.getId gets called. This returns the {id: "123123123"} object.
3. Post this we check the response is a plainObject and update the local storage to key: "123123123". Could you please let me know why this is done as this creates an inconsistency. On page reload, getId is no longer called but our script updates the value back to {id: "123123123"}. Hence storedId gets 2 different values. Initially it gets the string value, later it starts getting the object.
4. When we get an object, the createEidObject in file eids.js no longer is able to read the id as it expects a string. Implemented the getValue function to ensure that if we get an object read from it otherwise if it is a string (which prebid updates in local storage) return it as it is.
5. For now I have added the conditional check, and it resolves our problem of eids not being read. But why would we make an update to the local storage ? And if we need to do this can we not ensure that storedId is always a string in populateSubmoduleId ?

i think we probably need an engineer that knows the details of the module a bit better than myself (product manager) to help out here, @parthshah51999. i've requested that @dgirardi take a look to see if he can add some value.

The issue is that storage is meant to be managed by Prebid. On the other hand, I am not able to find a developer's guide for userId modules, so it's not surprising that this generates confusion.

This is the relevant piece of logic:

In the "normal" flow, line 769 does not find anything in storage, getId is called, and the return value's .id is then stored in line 789. On refresh, it branches off to extendId if present.

In your case, an external script populates the cookie, but in a different way (stringified JSON.stringify({id}) instead of just the id). getId is still called because some other pieces of metadata are missing (consentChanged). Then the cookie is overwritten in line 789, and on refresh it's not what you expect.

TL;DR how to fix?

The "correct" way is to not use storage configuration, if storage is not managed by Prebid, and instead read the cookie directly from the module. The issue with this mixed setup is that the userSync.userIds.storage configuration is meant to let the publisher choose when and how the ID is stored; but in your case, I don't think they have a choice, it needs to agree with what your script does, and as it is it just gives them an opportunity to get it wrong.

The easy fix is to update

to

    if (cacheIdObj?.id) return cacheIdObj;
    return cacheIdObj ? {id: cacheIdObj} : cacheIdObj;

but note that this will still overwrite your cookie, so if you store anything else, or want to manage expiration, you'll still have problems.

The issue is that storage is meant to be managed by Prebid. On the other hand, I am not able to find a developer's guide for userId modules, so it's not surprising that this generates confusion.

This is the relevant piece of logic:

Prebid.js/modules/userId/index.js

Lines 769 to 791 in fadc303

	let storedId = getStoredValue(submodule);
	let response;
	let refreshNeeded = false;
	if (typeof submodule.config.storage.refreshInSeconds === 'number') {
	const storedDate = new Date(getStoredValue(submodule, 'last'));
	refreshNeeded = storedDate && (Date.now() - storedDate.getTime() > submodule.config.storage.refreshInSeconds * 1000);
	}
	if (!storedId || refreshNeeded || forceRefresh || consentChanged(submodule)) {
	// No id previously saved, or a refresh is needed, or consent has changed. Request a new id from the submodule.
	response = submodule.submodule.getId(submodule.config, gdprConsent, storedId);
	} else if (typeof submodule.submodule.extendId === 'function') {
	// If the id exists already, give submodule a chance to decide additional actions that need to be taken
	response = submodule.submodule.extendId(submodule.config, gdprConsent, storedId);
	}
	if (isPlainObject(response)) {
	if (response.id) {
	// A getId/extendId result assumed to be valid user id data, which should be saved to users local storage or cookies
	setStoredValue(submodule, response.id);
	storedId = response.id;
	}

In the "normal" flow, line 769 does not find anything in storage, getId is called, and the return value's .id is then stored in line 789. On refresh, it branches off to extendId if present.

In your case, an external script populates the cookie, but in a different way (stringified JSON.stringify({id}) instead of just the id). getId is still called because some other pieces of metadata are missing (consentChanged). Then the cookie is overwritten in line 789, and on refresh it's not what you expect.

TL;DR how to fix?

The "correct" way is to not use storage configuration, if storage is not managed by Prebid, and instead read the cookie directly from the module. The issue with this mixed setup is that the userSync.userIds.storage configuration is meant to let the publisher choose when and how the ID is stored; but in your case, I don't think they have a choice, it needs to agree with what your script does, and as it is it just gives them an opportunity to get it wrong.

The easy fix is to update

Prebid.js/modules/deepintentDpesIdSystem.js

Lines 46 to 48 in fadc303

	getId(config, consentData, cacheIdObj) {
	return cacheIdObj;
	},

to

    if (cacheIdObj?.id) return cacheIdObj;
    return cacheIdObj ? {id: cacheIdObj} : cacheIdObj;

but note that this will still overwrite your cookie, so if you store anything else, or want to manage expiration, you'll still have problems.

Hey @dgiradi, thanks this works. Also noted that the cookie value is consistently overwritten post the fix. We are okay with this for now and will follow up if we need to make additional updates.

Quick question just for my understanding:
I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected. I did not understand the flow for this could you point me to the relevant piece of code so I can understand this better.

I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected

forceRefresh should not be needed for the flow to work, and whether it's set depends on your setup (in my case it isn't and the call happens once).

It's true when:

• the user explicitly asks for a refresh with refreshUserIds
  

  Prebid.js/modules/userId/index.js

  Lines 696 to 714 in 51bcf81

  	/**
  	* Force (re)initialization of ID submodules.
  	*
  	* This will force a refresh of the specified ID submodules regardless of `auctionDelay` / `syncDelay` settings, and
  	* return a promise that resolves to the same value as `getUserIds()` when the refresh is complete.
  	* If a refresh is already in progress, it will be canceled (rejecting promises returned by previous calls to `refreshUserIds`).
  	*
  	* @param submoduleNames? submodules to refresh. If omitted, refresh all submodules.
  	* @param callback? called when the refresh is complete
  	*/
  	function refreshUserIds({submoduleNames} = {}, callback) {
  	return initIdSystem({refresh: true, submoduleNames})
  	.then(() => {
  	if (callback && isFn(callback)) {
  	callback();
  	}
  	return getUserIds();
  	});
  	}

• a userId module is installed after it has already been configured
  

  Prebid.js/modules/userId/index.js

  Lines 1033 to 1042 in 51bcf81

  	export function attachIdSystem(submodule) {
  	if (!find(submoduleRegistry, i => i.name === submodule.name)) {
  	submoduleRegistry.push(submodule);
  	GDPR_GVLIDS.register(MODULE_TYPE_UID, submodule.name, submodule.gvlid)
  	updateSubmodules();
  	// TODO: a test case wants this to work even if called after init (the setConfig({userId}))
  	// so we trigger a refresh. But is that even possible outside of tests?
  	initIdSystem({refresh: true, submoduleNames: [submodule.name]});
  	}
  	}

Do you mind updating the unit tests? they are passing for all the wrong reasons:

getId is not affected by any of the preconditions (all the parameters provided by the tests are ignored, and the storage mocks also have no effect).

I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected

forceRefresh should not be needed for the flow to work, and whether it's set depends on your setup (in my case it isn't and the call happens once).

It's true when:

• the user explicitly asks for a refresh with refreshUserIds
  

  Prebid.js/modules/userId/index.js

  Lines 696 to 714 in 51bcf81

  	/**
  	* Force (re)initialization of ID submodules.
  	*
  	* This will force a refresh of the specified ID submodules regardless of `auctionDelay` / `syncDelay` settings, and
  	* return a promise that resolves to the same value as `getUserIds()` when the refresh is complete.
  	* If a refresh is already in progress, it will be canceled (rejecting promises returned by previous calls to `refreshUserIds`).
  	*
  	* @param submoduleNames? submodules to refresh. If omitted, refresh all submodules.
  	* @param callback? called when the refresh is complete
  	*/
  	function refreshUserIds({submoduleNames} = {}, callback) {
  	return initIdSystem({refresh: true, submoduleNames})
  	.then(() => {
  	if (callback && isFn(callback)) {
  	callback();
  	}
  	return getUserIds();
  	});
  	}

• a userId module is installed after it has already been configured
  

  Prebid.js/modules/userId/index.js

  Lines 1033 to 1042 in 51bcf81

  	export function attachIdSystem(submodule) {
  	if (!find(submoduleRegistry, i => i.name === submodule.name)) {
  	submoduleRegistry.push(submodule);
  	GDPR_GVLIDS.register(MODULE_TYPE_UID, submodule.name, submodule.gvlid)
  	updateSubmodules();
  	// TODO: a test case wants this to work even if called after init (the setConfig({userId}))
  	// so we trigger a refresh. But is that even possible outside of tests?
  	initIdSystem({refresh: true, submoduleNames: [submodule.name]});
  	}
  	}

Hey @dgirardi,

Yes the call happens only once. This led to more debugging from my end and it turns out to test a few things out I had explicitly called the refreshUserIds() function before the setConfig call from my test page.

Apologies for the miss on my end, but the solution you suggested is not working out.

1. In our case the client is trying to set this up to read from local storage instead of cookies.
2. After the storage gets updated due to line 789 like you mentioned, getId is no longer called and the code starts branching out to extendId.
3. Due to this, any updates we make to the getId function (like you suggested) are no longer useful.
4. This results in the eids not being populated on page refresh because of this check -

   Prebid.js/modules/userId/eids.js

   Line 12 in fadc303

   if (isStr(value)) {

   
   (as our script updates the Id back to (stringified JSON.stringify({id}))
5. Accepting the fact that prebid will continue to update the localStorage values there are 3 possible fixes that I saw:
   a. Implement the extendId method which will start returning a valid response so that storedId always gets a string value.
   b. Update the decode function called in line 801 and implemented in deepintentIdSystem to check if the the type of 'value' is a string/object and take appropriate action conditionally.
   c. What I have implemented to make sure that whenever we are trying to build eids, make sure the getValue function is implemented in config which will ensure that a string value is always read from the localStorage instead of an object. Something like this ref-

   Prebid.js/test/spec/modules/userId_spec.js

   Line 108 in fadc303

   getValue: function(data) {

Does that make sense? Let me know in case I am missing something.
Let me know if you want me to raise a separate PR for this considering the number of commits that have gone on this PR unnecessarily :p

You are right - my suggestion only works as long as you don't already have a cookie set.

I think (a) technically the best solution, but what you have should also work. (I was worried that it would only work for eids and not the "legacy" userId object, but reading through the code I think it happens to not matter).

You are right - my suggestion only works as long as you don't already have a cookie set.

I think (a) technically the best solution, but what you have should also work. (I was worried that it would only work for eids and not the "legacy" userId object, but reading through the code I think it happens to not matter).

I had implemented the extendId in the first iteration of this PR but this was suggested - #11383 (comment)

I can update the PR if necessary to implement the extendId function. But if there is no downside you foresee with this change this seems okay to me as well. Let me know how to proceed here.

This is OK, except for the tests - as I noted above, think most of the suite is not actually testing anything.

This is OK, except for the tests - as I noted above, think most of the suite is not actually testing anything.

Cool I will update the tests, thanks for your help!

@dgirardi @ChrisHuie any specific reason this was not picked for the 8.47.0 release ?

@dgirardi @ChrisHuie any specific reason this was not picked for the 8.47.0 release ?

Don't believe this pr was approved when I released Wednesday morning but merging now so it is included with this week's release.