Skip to content
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

Getting 2147483647 for all AdGroup->id returned by BatchJob mutateResult #236

Closed
jclee100 opened this issue Feb 10, 2017 · 24 comments
Closed
Assignees
Labels

Comments

@jclee100
Copy link
Contributor

2147483647 is the max of a 32 bit integer.

I am running batch jobs to create campaign with adGroups, ads and so on.

The batch job returns results via mutateResult but all AdGroups' ids in mutateResult are maxed out at 2147483647. Please tell me it's not just me! 😆

@fiboknacky fiboknacky self-assigned this Feb 10, 2017
@fiboknacky
Copy link
Member

Hello @komirad

Did you mean you assigned the negative temporary ID but in the real result, you got only 2147483647?
If so, could you please tell your batch job ID too?

Thanks!
Knack

@jclee100
Copy link
Contributor Author

Hi, I tried many times but the last BatchJob id is 444191858. Yes, I assigned a negative temp id.

If I am not wrong the XML returned is ok, but when accessing the mutateResult object, I get 2147483647.

I am doing more tests now.

Google\AdsApi\AdWords\v201609\cm\AdGroup {
  #id: 2147483647                         
  #campaignId: 753202213                  
  #campaignName: "Test 11"
...
}   

@jclee100
Copy link
Contributor Author

Yea I am guessing something went wrong during denormalizing.

I tried outputting from \Symfony\Component\Serializer\Serializer::denormalizeObject

The data seems ok here.

{
	"result": {
		"AdGroup": {
			"id": "36317301061",
			"campaignId": "753388736",
			"campaignName": "Test 4",
			"name": "product-46",
			"status": "ENABLED",
			"biddingStrategyConfiguration": {
				"biddingStrategyType": "MANUAL_CPC",
				"biddingStrategySource": "CAMPAIGN",
				"biddingScheme": {
					"@xsi:type": "ManualCpcBiddingScheme",
					"BiddingScheme.Type": "ManualCpcBiddingScheme",
					"enhancedCpcEnabled": "false"
				},
				"bids": [{
					"@xsi:type": "CpcBid",
					"Bids.Type": "CpcBid",
					"bid": {
						"ComparableValue.Type": "Money",
						"microAmount": "1000000"
					},
					"cpcBidSource": "ADGROUP"
				}, {
					"@xsi:type": "CpmBid",
					"Bids.Type": "CpmBid",
					"bid": {
						"ComparableValue.Type": "Money",
						"microAmount": "10000"
					},
					"cpmBidSource": "ADGROUP"
				}]
			}
		}
	},
	"index": "48"
}

@fiboknacky
Copy link
Member

Hello,

Thanks for reporting.
I will have a look.

Best,
Knack

@jclee100
Copy link
Contributor Author

Hi @fiboknacky,

Thanks. Do let me know if you can reproduce it.

I am stuck here as I need this to work to deploy my app.

JC

@fiboknacky
Copy link
Member

Hi JC,

It's probably due to this line.
We cast the returned value based on the type that it's supposed to be.

Unfortunately, intval() will return the value based on your system, which is 2147483647 in your case.
If urgent, could you please try the workaround stated here?

I'll discuss internally what we should do with this case.
Thanks!

Best,
Knack

@jclee100
Copy link
Contributor Author

@fiboknacky I think you are right. It's intval. I'll see if it works in my other dev machine running OSX.

@jclee100
Copy link
Contributor Author

Problem does not occur on OSX. Just Windows (7).

@fiboknacky
Copy link
Member

fiboknacky commented Feb 13, 2017

Thanks @komirad
BTW, did you get affected by this if you create an ad group or campaign by using SOAP services?
Do you get back an object whose ID is always 2147483647 in 32 bit system?

Knack

@jclee100
Copy link
Contributor Author

Hi @fiboknacky,

I haven't had the chance to try creating those using SOAP services. Will report back on that when it happens, but it's not going to happen anytime soon.

By the way, my win 7 is 64 bits. Maybe the software is running 32 bits? I am not sure how this works. 😃

@fiboknacky
Copy link
Member

Hi JC,

I think the latter--Probably your PHP on windows running 32 bit?
Anyway, we're talking about how to fix this in the long run.

Cheers,
Knack

@jclee100
Copy link
Contributor Author

Yup, looks like it.

\bin\apache\httpd-2.4.23-win32-VC14\bin\httpd.exe

@vtsao vtsao added the bug label Feb 13, 2017
@fiboknacky
Copy link
Member

This is fixed with v25.3.0.

jclee100 added a commit to jclee100/googleads-php-lib that referenced this issue Mar 3, 2017
jclee100 added a commit to jclee100/googleads-php-lib that referenced this issue Mar 3, 2017
Fix 32 bit integer issue (googleads#236) for v201609
@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

@fiboknacky

v201609 was not fixed in new release.

@fiboknacky
Copy link
Member

Hello JC,

As this is a deprecated branch, which will go away in months, I've fixed only the latest API version only.

Cheers,
Knack

@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

Hmm ok. I am still using v201609. So I was surprised to still get the same issue.

By the way if it's ok to ask usage question here. When migrating to a new version, eg, v201609 to v201702, I have to go into every file and change use Google\AdsApi\AdWords\v201609\cm\AdGroup; to use Google\AdsApi\AdWords\v201702\cm\AdGroup; Is there a better way to do this? There are so many files.

This wasn't a problem with the old library as version can be selected through composer (Kind of. By choosing which directory to load from).

Maybe release that only contains one version so that the version does not have to be in namespace?

@fiboknacky
Copy link
Member

Hello,

Have you considered using unix command "sed" or similar?
It should help you change everything from v201609 to v201702 quite easily.

For example, going to your project dir, and then run the below command:
sed -i 's/v201609/v201702/g' ./*/*
This should be finished within seconds. :)

Cheers,
Knack

@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

Hi,

Yes, I know I can also find and replace with my IDE. Just didn't seem like the most optimal way to handle this, having to change every file.

Thanks

@fiboknacky
Copy link
Member

Hmm..
The command will be run over every file for sure.
But you need to run this command only once. You don't need to run this command for all files by yourself.
Or do I understand your case incorrectly?

Knack

@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

Yes, I understand the command will change all instances in all files within the directory. Just as find and replace in path using IDE would. Ideally, there's no need to do this at all as there isn't really any code changes. Haven't encountered this with other libraries.

Another question: I have to serialize classes (eg. AdGroup) to either save or output as JSON in frontend. Since all the classes are now using protected properties, I have to use ReflectionClass to access and perform the serialization. Is there a more efficient way to do this?

Any chance of implementing something like JsonSerializable/toArray() or similar on the classes?

@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

Maybe the latest release should only contain the latest API version. Deprecated versions can still be patched in a separate branch. eg. 201609.25.3.0 :) There are pros and cons.

@fiboknacky
Copy link
Member

Hello,

Thanks for the feedback.
Yeah, I think there are pros and cons. Given that we have already deprecated and main branches, I think creating many branches like that would confuse people in some ways.
As I have seen other APIs, it's a bit different in that AdWords API libraries generate the classes from WSDL whereas other APIs would just have general classes and you specify API versions by passing parameters to those classes.
Many SDKs also have versions that follow their APIs quite closely and not by the date like AdWords API, so things are a bit less complicated. :)

As for your question, our (main branch) library already supports Symfony serializer and as you may notice, we also provide AdWordsNormalizer for this too.
Can you somehow apply that to serialize objects into JSON?
Theoretically it should be easy because we can successfully use it to serialize objects into XML.

Best,
Knack

@jclee100
Copy link
Contributor Author

jclee100 commented Mar 3, 2017

@fiboknacky

Thanks! Looks I may have to add a similar @xsi:type property into JSON to identify the class. I've been using the type enum properties already in the classes for that -- which requires manual mapping.

About doing the replace from v201609 to v201702. Is it safe to assume nothing will break?

@fiboknacky
Copy link
Member

Hi @komirad

About migrating from v201609 to v201702, I recommend you to have a look at our migration guide to see if there are any features you've used in v201609 that would need changes in v201702.

As for the library itself, the versioning follows semver closely, so if the major version doesn't change, you can rest assured that it's not breaking anything.

Finally, I'm truly sorry for my confusion.
The pull request #248 you've submitted is actually valid, especially as this is for the main branch, not the deprecated one.
If you still would like to send that PR for review please let me know.
But could you please add tests as we did in v201702 as well?

Best,
Knack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants