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

Add nl.basjes/yauaa_context/jsonschema/2-0-0 #1426

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oguzhanunlu
Copy link
Member

@oguzhanunlu oguzhanunlu commented Feb 25, 2025

This PR upgrades yauaa_context schema to 2-0-0 by converting enums to strings with a future-proof maxLength(128). We don’t have control on those enums and any new version of yauaa lib can add and remove items.

Why bump to 2-0-0?
coming soon

$ diff schemas/nl.basjes/yauaa_context/jsonschema/1-0-4 schemas/nl.basjes/yauaa_context/jsonschema/2-0-0
8c8
<         "version": "1-0-4"
---
>         "version": "2-0-0"
14c14,15
<             "enum": ["Desktop", "Anonymized", "Unknown", "UNKNOWN", "Mobile", "Tablet", "Phone", "Watch", "Virtual Reality", "eReader", "Set-top box", "TV", "Game Console", "Home Appliance", "Handheld Game Console", "Voice", "Car", "Robot", "Robot Mobile", "Spy", "Hacker", "Augmented Reality", "Robot Imitator"]
---
>             "type": "string",
>             "maxLength": 128
45c46
<             "enum": ["Desktop", "Mobile", "Cloud", "Embedded", "Game Console", "Hacker", "Anonymized", "Unknown"]
---
>             "maxLength": 128
67c68
<             "enum": ["Browser", "Mobile App", "Hacker", "Robot", "Unknown", "Special", "Cloud", "eReader"]
---
>             "maxLength": 128
96c97
<             "enum": ["Browser", "Browser Webview", "Mobile App", "Robot", "Robot Mobile", "Cloud Application", "Email Client", "Voice", "Special", "Testclient", "Hacker", "Unknown", "Desktop App", "eReader"]
---
>             "maxLength": 128
139c140
<             "enum": ["Weak security", "Strong security", "Unknown", "Hacker", "No security"]
---
>             "maxLength": 128
153a155,158
>         "webviewAppNameVersion": {
>             "type": "string",
>             "maxLength": 1000
>         },

ref: PDP-1090

@oguzhanunlu oguzhanunlu self-assigned this Feb 25, 2025
@oguzhanunlu oguzhanunlu marked this pull request as draft February 25, 2025 15:15
@oguzhanunlu oguzhanunlu marked this pull request as ready for review February 26, 2025 12:57
Copy link
Contributor

@istreeter istreeter left a comment

Choose a reason for hiding this comment

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

My only doubt is whether 32 is long enough to future-proof the new fields.

In #1216 we got bored of forever bumping max lengths, whenever the yauaa library changed, or whenever a new user agent got invented. So we deliberately bumped all max lengths to 128 or more, to better future-proof the schema.

Copy link
Contributor

@istreeter istreeter left a comment

Choose a reason for hiding this comment

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

Looks good from my point of view.

@oguzhanunlu please can you remind me why this is 2-0-0, not 1-0-5? I'm sure you're probably right, but I've forgotten the reason.

Even better... you could add the explanation to the PR description, so we have a long term reference of why it is 2-0-0.

How urgently does this need to be merged? Presumably you can you use the dev registry while working on the feature in Enrich?

@oguzhanunlu
Copy link
Member Author

oguzhanunlu commented Feb 27, 2025

Thanks @istreeter ! I just updated PR description with a short note on why it is 2-0-0 from my point of view. There is no urgency as I can reference my branch when needed. Let me add that this is a blocker for the next enrich release which is coming in the next few weeks.

@istreeter
Copy link
Contributor

istreeter commented Feb 27, 2025

payloads of new schema will always be invalid against 1-0-4, hence a model bump.

This is not the criteria for a model bump. We often add new extra fields to iglu central schemas, and we do it as an addition (e.g. 1-0-0 --> 1-0-1). The original schemaver blog post tells us so.

Now for this PR.... you might be correct it should be a 2-0-0 schema, but it's not because you added the webviewAppNameVersion field. It's because you changed several fields from enum to regular string. If there is any chance that might break any loader, then this should be a 2-0-0 schema. That is the point I want to see documented in this PR description.

@oguzhanunlu
Copy link
Member Author

thanks @istreeter ! my bad, I was asking the wrong question. The blog post asks Will this new schema validate all historical data?. Here all historical data will be valid against new schema as the new field isn't required and additionalProperties is false, so it indicates an addition on its own.

It's because you changed several fields from enum to regular string. If there is any chance that might break any loader, then this should be a 2-0-0 schema.

Asking the same question. Since previous enum possibilites are always string with length <128, it looks like new schema can validate all historical data. So it indicates an addition as well. However I'm not sure if type conversion could break some of the loaders, I need to check them.

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

Successfully merging this pull request may close these issues.

2 participants