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

feat(server): Nullable asset dates #15669

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(server): Nullable asset dates #15669

wants to merge 3 commits into from

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Jan 25, 2025

Allow fileCreatedAt, fileModifiedAt, localDateTime to be null, making them placeholders before metadata extraction runs

No dtos have changed, so clients don't need to be updated

@etnoy etnoy force-pushed the feat/nullable-dates branch 3 times, most recently from f5f4059 to 52ba6fa Compare January 26, 2025 00:13
@etnoy etnoy force-pushed the feat/nullable-dates branch 4 times, most recently from bc9702a to 50de167 Compare January 26, 2025 22:35
@etnoy etnoy marked this pull request as ready for review January 26, 2025 22:45
@etnoy etnoy requested a review from danieldietzler as a code owner January 26, 2025 22:45
@etnoy etnoy force-pushed the feat/nullable-dates branch 2 times, most recently from 3a1c659 to 3723484 Compare January 27, 2025 22:33
@etnoy etnoy requested a review from mertalev as a code owner January 27, 2025 22:33
@github-actions github-actions bot added documentation Improvements or additions to documentation 📱mobile labels Jan 27, 2025
@etnoy etnoy force-pushed the feat/nullable-dates branch from 3723484 to fd823d2 Compare January 27, 2025 22:33
@etnoy etnoy force-pushed the feat/nullable-dates branch from fd823d2 to 3b4276f Compare January 27, 2025 22:39
@etnoy etnoy force-pushed the feat/nullable-dates branch 6 times, most recently from 71fc410 to e213a74 Compare January 28, 2025 02:51
@alextran1502 alextran1502 requested a review from jrasm91 January 28, 2025 03:55
@alextran1502
Copy link
Contributor

Hello, how has this PR been tested?

Is there a better way to always group those column is not null conditions to a helper method?

@mertalev
Copy link
Contributor

Any query that does an inner join on the exif table can do without those filters. The ones that don't only really need to check one of those columns since they're all set at the same time anyway (i.e. they will all be null or all non-null).

Comment on lines +116 to +122
if (!entity.localDateTime) {
throw new Error('Asset localDateTime is missing');
} else if (!entity.fileCreatedAt) {
throw new Error('Asset fileCreatedAt is missing');
} else if (!entity.fileModifiedAt) {
throw new Error('Asset fileModifiedAt is missing');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mapAsset being used in a context where this can happen is a bug. It's better to handle this with stricter typing in the input, both to catch issues before runtime and to avoid unnecessary runtime cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to find a better solution for this, I agree it isn't very good.

Comment on lines +66 to +68
withNullLocalDateTime?: boolean;
withNullFileCreatedAt?: boolean;
withNullFileModifiedAt?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the searchAssetBuilder, see my code in asset.entity.ts. Maybe we need to find assets that have this as null in the future?

Otherwise, if you think it's better, I'll just add a non-null where parameter for each of the nullable dates that can't be turned off.

Copy link
Contributor

@mertalev mertalev Jan 28, 2025

Choose a reason for hiding this comment

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

We can go without this and always set the filter until there's a use-case for these toggles.

server/src/services/metadata.service.ts Outdated Show resolved Hide resolved
@etnoy
Copy link
Contributor Author

etnoy commented Jan 28, 2025

Any query that does an inner join on the exif table can do without those filters. The ones that don't only really need to check one of those columns since they're all set at the same time anyway (i.e. they will all be null or all non-null).

Thanks, I'll modify the code accordingly

@etnoy etnoy force-pushed the feat/nullable-dates branch from e213a74 to b632b84 Compare January 28, 2025 12:47
@etnoy
Copy link
Contributor Author

etnoy commented Jan 28, 2025

Hello, how has this PR been tested?

Is there a better way to always group those column is not null conditions to a helper method?

Currently I've done some superficial manual tests. I wanted to get some feedback from the team before diving deeper.

In this PR, no asset will ever actually have a null date. All API endpoints require these dates to be set, it's only in the backend.

In my subsequent PR, #14456 , the changes done here are included. In that PR we actually have null dates since the library service sets crawled assets to have null dates as placeholders until metadata extraction runs. In that case I've done more testing, doing library imports, manual uploads and so on.

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

Successfully merging this pull request may close these issues.

4 participants