feat: Ensure service account file exists before initializing credentials

[cacheflow]

This PR adds a validation check to ensure that the service_account_file path passed to FCM exists before attempting to initialize credentials using Google OAuth.

References #385.

[cacheflow]

Hey @olucurious or @Niccari when y'all have a moment, can you review this PR?

[Niccari]

Thanks for the fix! I've reviewed it and here are my comments below. I've also prepared a suggestion branch (https://github.com/Niccari/PyFCM/tree/fix-suggestion-ensure-service-account-file-exists) — feel free to pull it in if it looks good.

[Niccari]

{path} here refers to the os.path module imported above, not the file path string. This will render as something like <module 'posixpath' from '...'> in the error message. It should be {self._service_account_file}.

Suggested change

	raise InvalidDataError(f"The service account file you passed does not exist at '{path}'. "
	if not path.isfile(self._service_account_file):
	raise InvalidDataError(
	f"The service account file you passed does not exist at '{self._service_account_file}'. "
	"Ensure it does not have any typos and exists."
	)

[cacheflow]

Thanks for the feedback. I've incorporated the change.

[Niccari]

This looks like a leftover note. Could you remove it before merging? (FYI, the test does run — notify() calls send_request → request_headers → _get_access_token → _initialize_credentials.)

Suggested change

	# figure out why this goddamn test is not running
	fcm = FCMNotification(service_account_file='./foo.json', project_id=None, credentials=None)

[cacheflow]

Done! Sorry about that. 😆

[Niccari]

Thanks for the updates — the error message and the comment are both addressed now. However, this revision introduces a runtime regression I'd like to flag before merge. See the inline comment.

I've updated my suggestion branch (https://github.com/Niccari/PyFCM/tree/fix-suggestion-ensure-service-account-file-exists), rebased on this PR's latest commit (see: 08041af), which restores the correct class and adds a regression test. Feel free to pull it in.

[Niccari]

This is a runtime regression. Credentials here is google.oauth2.credentials.Credentials (the user/OAuth2 access-token class), which has no from_service_account_file classmethod. With a valid service_account_file, this raises:

AttributeError: type object 'Credentials' has no attribute 'from_service_account_file'

Only google.oauth2.service_account.Credentials provides from_service_account_file, which is why the original code imported and used service_account. The removal of from google.oauth2 import service_account (line 11) needs to be reverted as well.

Suggested change

	self.credentials = Credentials.from_service_account_file(
	self.credentials = service_account.Credentials.from_service_account_file(

[cacheflow]

You're right. I saw two credential files in the upstream Google lib and pulled in the wrong one.

[cacheflow]

Thanks again. I've brought in the changes from your branch.

[Niccari]

Thanks, LGTM! 🎉

note - As a non-maintainer reviewer I don't have permission to trigger it.
so @olucurious would need to approve the workflow run to get CI to execute.

[cacheflow]

Hey @olucurious, do you think I could get another set of eyes on this? I contributed a similar change to the Ruby version of FCM a while back and wanted to do the same for the Python package: decision-labs/fcm#134

[cacheflow]

Hello @olucurious I noticed that you closed and re-opened the PR. Is there anything else I can provide on my end to help get this PR reviewed?

[olucurious]

Hi @cacheflow, thanks for following up, and sorry for the confusing close/reopen notification. That was only done to retrigger GitHub Actions.

I’ve now reviewed the updated implementation and CI is passing. I found one issue to address before merging: the new error message includes the complete value of service_account_file. If someone accidentally passes raw service-account JSON instead of a path, the exception could expose credentials in application logs.

Could we change the message so it doesn’t interpolate the supplied value? For example:

raise InvalidDataError(
"The service account file does not exist or is not a regular file."
)

It would also be useful to add a regression test confirming that credential-like input is not included in the exception message. Nothing else is blocking from my review.

Thanks again for the contribution!