fix: avoid possible job already exists error

[plamut]

Fixes #738.

If job create request fails, a query job might still have started successfully. This PR handles this edge case and returns such
query job one can be found.

Based on the similar fix in the Java client.

PR checklist:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[plamut]

@tseaver I don't know the exact mechanics on the backend, this fix is mostly based on a similar fix in the Java client.

@tswast Can you chime in?

[plamut]

The docs check failure does not seem to be related:

gpg: keyserver receive failed: No name

Update: Indeed, but the fix is on its way.

[tseaver]

googleapis/synthtool#1155 landed here in #762. I'm not sure why the config isn't making you merge with master to pick that fix up, however.

[andrewjw]

Hi, there is a slight problem with this change - self.get_job has a different return type to this function. It can return LoadJob, etc as well as the QueryJob we're expecting so the actual return type doesn't match what is declared for this function.

I don't understand the situations that could result in this code being called, but presumably in reality this would always be a QueryJob? Unfortunately this is causing me problems when running pylint over some code that calls this, because it thinks the function can return LoadJob, and that has a different set of members to QueryJob.

Many thanks,
Andrew Wilkinson

[plamut]

Indeed, in this context self.get_job() returns a QueryJob, because job_id is the same ID that was used a few lines above when constructing a new query job (and then starting it).

This project uses pytype for static type checks and it did not complain, but apparently pylint could not deduce the same and reported a false issue.

Could you tell pylint to ignore return type in that specific line where query() is called? IMHO that justifiable, because pylint is wrong there.

[andrewjw]

Having looked into this a bit further I agree that pylint is wrong. It's a bit of a pain to have disable this check every time we call query, but I think this is a sign that pylint is aging and not keeping up with modern Python's type syntax.

Sorry for the noise.

Cheers,
Andrew

[plamut]

No worries, it was a perfectly valid comment.

Ideally, pylint would allow ignoring particular warnings for lines matching a regex, but I'm not sure if that's currently supported? It would make disabling those false positives much cleaner compared to spamming the # pylint: disable=... comments all around the code.

[andrewjw]

Sadly the error isn't raised on the call to query, but when you try and use the return value. In my case this is accessing num_dml_affected_rows, which only exists on QueryJob, and not on LoadJob. Even if it did support disabling errors using a regex, I'm not sure it would be practical to create one.

It's been bugging me why this wouldn't be picked up by the type checker. I think I've tracked it down to the fact that LoadJob, QueryJob, etc all derive from _AsyncJob, which in turn derives from google.api_core.future.polling.PollingFuture. The problem is that google.api_core.future.polling.PollingFuture is not typable, so it gets turns into an Any type, which makes all the job types equivalent and therefore doesn't generate an error. When testing with mypy you have to add # type: ignore to the PollingFuture import line explicitly, but I guess pytype is more forgiving.

I've create the attached file demonstrating the problem (annoyingly github won't let me attach the file as a .py). As currently written it'll generate an error in both mypy and pytype, but swap the comments on lines 5 and 6 and the error goes away.

Anyway, I have a reasonable workaround, so if you want to leave this that's absolutely fine. If in future the python-api-core library adds typing then I expect this to break though. Adding an assert isinstance(query_job, job.QueryJob) will resolve the issue.

Cheers,
Andrew
invalid_return_union.txt