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

GCM/FCM device uuid #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewh
Copy link
Collaborator

@matthewh matthewh commented Apr 17, 2017

Replace HexIntegerField with UUIDField. Allows backwards compatibility through the REST API by left zero-padding values.

Resolves #358

@matthewh
Copy link
Collaborator Author

matthewh commented Apr 17, 2017

@jleclanche I'm on the fence about making the GCM device_id a UUID. I think a CharField that can accommodate a UUID would be more appropriate for backwards compatibility.

The guidelines indicate a UUID is appropriate but I don't have enough experience with the alternatives. For the device_id is to be useful when sending messages with a QuerySet then we shouldn't mess with the sent value too much or the developer will need to remember to translate the device_id to the appropriate format. Seems error prone.

@jleclanche
Copy link
Member

That page you linked talks about creating a random uuid.

I think it's fine, tbh, but the main things that need to be tested are how the DRF API reacts to it, and how the migration holds up on an existing db.

@matthewh matthewh force-pushed the feature/gcm_device_id branch 2 times, most recently from fd6093c to a07fd62 Compare April 17, 2017 19:25
@matthewh
Copy link
Collaborator Author

@jleclanche If we adopt UUIDs (and the provided UUIDField for storage and serialization) there is no way to support old data, even with a migration. Existing GCM apps would have to update their logic to send UUIDs because the UUIDFields expect full uuid values. To maintain backwards compatibility, and for the migrations data to work, we must provide a custom serialization uuid field that can accept the old values and left pad them before they are saved to the database.

@jleclanche
Copy link
Member

A data migration is all that's needed. Left-padding isn't necessary, UUIDs are just 128-bit ints. What's the issue exactly?

@matthewh
Copy link
Collaborator Author

matthewh commented Apr 19, 2017

Django's UUIDField doesn't accept some values we could expect from Android apps that aren't using UUIDs.

Existing installations are likely to try values like 0xdeadbeaf and fail without a custom UUIDField in DRF. Do you accept we must maintain a custom UUIDField in DRF?

		# device_id is optional - success
		device = GCMDevice.objects.create(
			registration_id="foobar",
			device_id=None
		)

		# short values not supported - raises ValueError
		with self.assertRaises(ValueError) as raised:
			device = GCMDevice.objects.create(
				registration_id="foobar",
				device_id=123456
			)

		# short values not supported - raises ValueError
		with self.assertRaises(ValueError) as raised:
			device = GCMDevice.objects.create(
				registration_id="foobar",
				device_id="0xdeadbeaf"
			)

@jleclanche
Copy link
Member

Are you talking about specifically DRF? If so, yes, I'm fine with us using a custom serializer wrapping UUIDField.

@ddemid
Copy link

ddemid commented Sep 18, 2019

Is this going to be ever merged? it's seems like a valid solution

@acenario
Copy link

acenario commented Mar 2, 2021

Bump!

Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

this needs rebase

@piyushgupta27
Copy link

Is there any update on the same? We currently are not able to support iOS notifications via FCM. I can see that the fix is there but has been pending for a long time now?

@azmeuk azmeuk added the gcm/fcm label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace HexIntegerField with a CharField
7 participants