-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(server): adjust type of person.birthDate #16628
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
fix(server): adjust type of person.birthDate #16628
Conversation
968fd25 to
3308d08
Compare
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.
I edited this file manually. Whenever I tried to update it automatically, it would generate a large diff unrelated to my changes, some of which would cause compilation errors. Is this a known issue?
jrasm91
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.
We want it to parse dates automatically I think. It's a feature we've enabled on purpose. We also already have type mappings (like this) in the config.repository.ts file. I think the fix should be specifically in the person.dto mapper function
How did I miss those... I was going crazy to trying to figure out why the e2e tests were failing. Thanks for the info, I'll amend my PR. |
024b207 to
52294f1
Compare
e119cfe to
7595b81
Compare
server/src/dtos/person.dto.ts
Outdated
| hasNextPage?: boolean; | ||
| } | ||
|
|
||
| const asDateString = (x: string | Date | null) => (x instanceof Date ? x.toISOString().split('T')[0] : x); |
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.
Is this a dupe of the utils/date.ts below?
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.
My bad, fixed
The API currently does not respect the documentation when returning a person's birthDate. The doc/swagger says it will be of "YYYY-MM-DD" format but the string is a full ISO8601-with-tz string. This causes issue immich-app#16216 because the <input> tag is strict about supported value formats. I believe this was introduced by immich-app#15242 which switched some queries from TypeORM to Kysely for the person repository. TypeORM does not parse date, but our Kysely configuration does (explicitely). This commits updates the types to represent both possibilities and ensure the API always returns the correct format.
7595b81 to
2c33f19
Compare
…s-broken * main: fix(server): set the dev server restart policy of the dev server container to match the other containers (immich-app#16753) feat(web): remember search context (immich-app#16614) feat(server): read Android and Sony video camera make/model (immich-app#16678) fix(server): adjust type of person.birthDate (immich-app#16628) fix(web): add labels to memory lane buttons (immich-app#16664) feat(mobile): locate in timeline (immich-app#16722) chore(ml): uv (immich-app#16725) fix: 🍪 packages confusion (immich-app#16735) fix(web): Update people-card favorite position (immich-app#16746) chore(mobile): upgrade riverpod (immich-app#16742) chore(mobile): upgrade flutter_web_auth_2 (immich-app#16741) fix(docs): edge case when restoring dump that is unreadable as current user (immich-app#16758)
The API currently does not respect the documentation when returning a person's birthDate. The doc/swagger says it will be of "YYYY-MM-DD" format but the string is a full ISO8601-with-tz string. This causes issue immich-app#16216 because the <input> tag is strict about supported value formats. I believe this was introduced by immich-app#15242 which switched some queries from TypeORM to Kysely for the person repository. TypeORM does not parse date, but our Kysely configuration does (explicitely). This commits updates the types to represent both possibilities and ensure the API always returns the correct format.
Description
The API currently does not respect the documentation when returning a person's birthDate. The doc/swagger says it will be of "YYYY-MM-DD" format but the string is a full ISO8601-with-tz string. This causes issue #16216 because the tag is strict about supported value formats.
I believe this was introduced by #15242 which switched some queries from TypeORM to Kysely for the person repository. TypeORM does not parse date, but our Kysely configuration does (explicitely).
This commits updates the types to represent both possibilities and ensure the API always returns the correct format.
Fixes #16216
How Has This Been Tested?
person/:id/(for a person with an age)photo/:id/(for an asset with a face)Screenshots (if appropriate)
API Changes
/person/:id/now correctly returnsbirthDateasYYYY-MM-DDinstead of full ISO8601-with-tzChecklist:
src/servicesuses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services)