Name the usage a 51Did was created for, and take the raw byte surface off FodId - #76
Open
jwrosewell wants to merge 2 commits into
Open
Name the usage a 51Did was created for, and take the raw byte surface off FodId#76jwrosewell wants to merge 2 commits into
jwrosewell wants to merge 2 commits into
Conversation
FodId exposed the flags byte and a type read from bits 6-7, and nothing for the usage in bits 0-2, so a caller deciding whether an identifier may be passed to a demand source had to mask the byte by hand. The three usages are cumulative in the byte, non-marketing 0b001, standard 0b011 and personalized 0b111, so a caller masking for the non-marketing bit alone would read every marketing identifier as non-marketing, the wrong way round for a data protection decision. Usage names the four states, NONE where no bit is set, and the usage property answers with the highest granted. usage_from_consent reads bit 3, which records that the usage came from a consent string rather than being stated. Usage.id_usage gives the cloud's id.usage value. The names are the same in every 51Did package, which all gain the accessor together. Found by the Trusted Server work, which must work only through this package. fiftyone_pipeline_did tests: 147 passed, 2 skipped, two new.
A 51Did could be read in two ways, being a typed accessor for each field and the raw byte with the offsets to read the same thing by hand. The second way is the one a caller is told never to use, and it is the way that produces the mistake the usage accessor was added to stop, because the usage bits are cumulative rather than exclusive, so a caller masking the byte for the non-marketing bit reads every marketing identifier as non-marketing, which is exactly backwards for a rule that says a non-marketing identifier must never be passed to a demand source. These packages are not widely deployed, so the raw surface is removed now rather than deprecated, and the same removal is made in every language so that the surface stays the same across all of them. Gone from FodId are the raw flags byte, the deprecated hash alias and the HASH_OFFSET and HASH_LENGTH constants left over from the match key rename, and the nine byte layout constants FLAGS_OFFSET, LICENSE_ID_OFFSET, LICENSE_ID_LENGTH, MATCH_KEY_OFFSET, MATCH_KEY_LENGTH, HEADER_LENGTH, GUID_LENGTH, RANDOM_PAYLOAD_LENGTH and PAYLOAD_LENGTH. The nine constants move to the new private module _layout, which the package and its own tests import and which is not exported from the package __init__, because the tests and the client build and measure payloads byte by byte and still need them. The flags byte itself stays on the instance as _flags, which is where the typed accessors read it and where the tests that pin the exact byte now read it. The raw date minutes goes too. No caller in any of the seven 51Degrees repositories used it, Rust never had it, and the one documented purpose, being the OWID public key request, is built by the 51Did client itself. The Python client did read it internally to pick the signing key, so that reading is now a private helper, _date_minutes in fod_id, and no longer a property anyone outside the package can reach. The tests that asserted on the minutes assert the same fact through the typed date. Every typed accessor is unchanged, being type, usage, usage_from_consent, license_id and match_key, plus the OWID level fields. The readme drops the byte layout table and the removed accessors from its example, gains a section on the usage accessor, and points at the specification for the layout and the package surface, as the fod_id docstrings now do. Closes #77
jwrosewell
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to the same surface, in the order they were made.
1. Name the usage a 51Did was created for
FodIdgainsusage(None, NonMarketing, Standard, Personalized) andusage_from_consent, reading bits 0 to 2 and bit 3 of the flags byte. The usage bits are cumulative, so the accessor answers the highest usage granted rather than leaving callers to mask bits and misread every marketing identifier as non-marketing. Same accessor, same names, in all six 51Did packages. Found by the Trusted Server work, which must work only through this package.2. Take the raw byte surface off FodId
This is issue 77, raised on all six packages so the surface stays the same in every language. Once every bit has a name, the raw byte and the offsets are the remaining way to get it wrong, so they go.
flagsproperty is removed. The byte stays as_flags, which the typed accessors read and which the tests that pin the byte now read.hash,HASH_OFFSETandHASH_LENGTHare deleted, with the alias tests and thewarningsimport they needed._layout.py, which is not exported from__init__.date_minutesis removed.did_client.pydid use it internally to pick the signing key, so it is now the private helper_date_minutes.DATE_EPOCHis unchanged.test_raw_surface_is_not_public, asserts that all fourteen removed names are absent from the class and from an instance.The example built its payload from the class constants, so it now states its own sample layout locally, which is right for a consumer, and prints the usage, its
id.usagestring and the consent bit instead of the raw byte.The open question, decided
The issue left the raw date minutes as a candidate rather than a definite. It goes as a public accessor in all five packages that have it. No caller outside the packages used it and Rust never had one. Python is the case where the client itself needed the value, so it keeps it privately.
The specification
The byte structure is now specified once for all six languages, on specifications 25, at Identifier layout with the surface each package exposes at Package surface. The class docstring and the README link there instead of repeating the tables. Those links resolve once that pull request merges.
Verification
The count was 147 before, and the one fewer is the deleted alias test. Running the tests needs the
owid-pythonsubmodule andci/copy-owid-source.ps1, which writes the ignored_owidpackage, and neither is committed.flake8reports the same twenty pre-existing findings as it does on the unchanged branch point, none of them in the changed lines. This repository's continuous integration runs no linter for the Python packages, onlytox -e py.Closes #77
Produced with AI assistance and needs human review.