-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add license information to the api #1482
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
Conversation
|
Here is an example of the /v1/feeds/{id} call: And for the /v1/licenses/{id} call (Updated after commit 1b21765) |
|
Example of /v1/licenses (Updated after commit 1b21765) |
|
Wonder what purpose |
|
*Lighthouse ran on https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app/ * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app/feeds * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app/feeds/gtfs/mdb-2126 * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app/feeds/gtfs_rt/mdb-2585 * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app/gbfs/gbfs-flamingo_porirua * (Desktop)
|
|
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-1482-e7wegjpi.web.app |
@emmambd this just reflects the data in the database. I guess the license_notes are specific to the feed, maybe adding info on how the license was obtained, or how it applies to that feed? |
| @@ -0,0 +1,56 @@ | |||
| name: Verify TypeScript GBFS Validator Types Generation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @Alessandro100
| lic_dict = { | ||
| "id": lic.id, | ||
| "type": lic.type, | ||
| "is_spdx": lic.is_spdx, | ||
| "name": lic.name, | ||
| "url": lic.url, | ||
| "description": lic.description, | ||
| "license_rules": [r.name for r in getattr(lic, "rules", [])], | ||
| "created_at": lic.created_at, | ||
| "updated_at": lic.updated_at, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this to the models folder, and it will avoid repetition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added license_base_impl.py and license_with_rules_impl.py
| # Determine license_is_spdx from the related License ORM if available | ||
| license_is_spdx = None | ||
| try: | ||
| if getattr(feed, "license", None) is not None: | ||
| license_is_spdx = feed.license.is_spdx | ||
| except Exception: | ||
| # be conservative and keep None if anything goes wrong | ||
| license_is_spdx = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip the try/except clause here.
| # Determine license_is_spdx from the related License ORM if available | |
| license_is_spdx = None | |
| try: | |
| if getattr(feed, "license", None) is not None: | |
| license_is_spdx = feed.license.is_spdx | |
| except Exception: | |
| # be conservative and keep None if anything goes wrong | |
| license_is_spdx = None | |
| # Determine license_is_spdx from the related License ORM if available | |
| license_is_spdx = None | |
| if getattr(feed, "license", None) is not None: | |
| license_is_spdx = feed.license.is_spdx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/DatabaseCatalogAPI.yaml
Outdated
| type: boolean | ||
| example: true | ||
| license_notes: | ||
| description: TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a generic text notes field. We can document it with:
| description: TBD | |
| description: Notes associated with the feed license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
docs/DatabaseCatalogAPI.yaml
Outdated
| example: 0BSD | ||
| type: | ||
| type: string | ||
| description: TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's connect to go over all pending TBD documention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all TBD values and put something that seemed correct.
| "name": lic.name, | ||
| "url": lic.url, | ||
| "description": lic.description, | ||
| "license_rules": [r.name for r in getattr(lic, "rules", [])], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion]: We can expose here the name and the label of each rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| return cls( | ||
| **base_license.model_dump(), | ||
| license_rules=rules or None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion]: return an empty array instead of none, it's safer to parse on the consumer side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this one. The function is not supposed to return a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confussion, I'm talking about the license_rules
davidgamez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary:
closes #1345
Added some license related fields to the feeds response, namely:
Added the /v1/licenses endpoint (see examples below).
Still to be done:
Return the
content_txtandcontent_htmlin a separate endpoint call. Suggestion:Add a
include_license_contentparameter to the/v1/licenses/{id}endpoint.license_rules part of the response
/v1/licensescontains the name of the rule. Maybe we want to use the label insteadlicense_rules could have its own endpoint?
AI summary
This pull request adds full support for managing and exposing license information for feeds in the API. It introduces new endpoints for license retrieval, updates feed models and test data to include license references, and ensures license relationships are efficiently loaded from the database. The changes also include new integration tests to verify license functionality.
Licenses API and Model Integration
LicensesApiImpland OpenAPI generator files. [1] [2] [3] [4] [5]Feed Model and Query Enhancements
BasicFeedImpl.from_ormto expose license fields (license_id,license_is_spdx,license_notes) in API responses. [1] [2]license_idandlicense_notesfrom test data, handling missing/empty values gracefully.Testing and Documentation
Testing tips:
On top of the random queries listed below, added some integration tests.
Provide tips, procedures and sample files on how to test the feature.
Testers are invited to follow the tips AND to try anything they deem relevant outside the bounds of the testing tips.
Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.shto make sure you didn't break anything