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

Added software_titles unique index idx_unique_sw_titles #25794

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

ksykulev
Copy link
Contributor

@ksykulev ksykulev commented Jan 27, 2025

For #25235

This allows software with different names but the same bundle identifier
to be grouped under the same title. It also allows for software with the
same name but different bundle identifiers to be under two separate
titles.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Added/updated automated tests
  • A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
  • Manual QA for all new/changed functionality

@iansltx
Copy link
Member

iansltx commented Jan 27, 2025

What happens if I have "GoLand 2.app" and "GoLand.app" both with the same bundle ID? Those should resolve to the same software title. Can chat through in tmw afternoon's meeting.

Also not 100% on collations, since we have #25353. Likely worth bringing this proposal to tomorrow's backend sync to get another set of eyes on this.

@ksykulev
Copy link
Contributor Author

ksykulev commented Jan 27, 2025

I can write an explicit test for "GoLand 2.app" and "GoLand.app" both with the same bundle ID. But this doesn't seem like it worked before or after this change.

@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from 73ddb20 to 02dd421 Compare January 28, 2025 02:37
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from 02dd421 to 44ce244 Compare January 28, 2025 02:54
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from 44ce244 to d63d96d Compare January 28, 2025 19:10
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from d63d96d to f6b9adb Compare January 28, 2025 19:33
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from f6b9adb to a38da6b Compare January 28, 2025 20:43
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from a38da6b to b813545 Compare January 28, 2025 21:02
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from b813545 to b794db2 Compare January 28, 2025 21:14
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from b794db2 to c56628c Compare January 28, 2025 21:22
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 69.44444% with 11 lines in your changes missing coverage. Please review.

Project coverage is 63.67%. Comparing base (49b0dcf) to head (42684f5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../20250124194347_UpdateSoftwareTitlesUniqueIndex.go 56.00% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25794   +/-   ##
=======================================
  Coverage   63.67%   63.67%           
=======================================
  Files        1628     1629    +1     
  Lines      156151   156183   +32     
  Branches     4051     4051           
=======================================
+ Hits        99425    99447   +22     
- Misses      48890    48897    +7     
- Partials     7836     7839    +3     
Flag Coverage Δ
backend 64.49% <69.44%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from c56628c to 175088a Compare January 28, 2025 23:16
@ksykulev ksykulev changed the title Allow software titles for software named the same but with different bundle identifiers Updated software_titles unique index idx_sw_titles to include bundle identifier when present Jan 28, 2025
@ksykulev ksykulev marked this pull request as ready for review January 28, 2025 23:17
@ksykulev ksykulev requested a review from a team as a code owner January 28, 2025 23:17
@ksykulev ksykulev changed the title Updated software_titles unique index idx_sw_titles to include bundle identifier when present Updated software_titles unique index idx_sw_titles to use bundle identifier when present Jan 28, 2025
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from 175088a to b0457a1 Compare January 28, 2025 23:19
Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

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

Maybe out of scope but another thing I noticed when troubleshooting for a customer related to this is a bunch of software entries with title_id values that were set but that didn't actually reference software_titles. I'm guessing the fkey was left off for performance reasons, but for sanity we should probably clean those up in ReconcileSoftwareTitles before the "clean up orphaned software titles" step, unless I'm missing something.

@mna @gillespi314 y'all touched the orphaned cleanup code most recently; is there a downside to removing invalid title_id refs where I'm mentioning?

@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from b0457a1 to 05b41a5 Compare January 29, 2025 22:23
@ksykulev ksykulev changed the title Updated software_titles unique index idx_sw_titles to use bundle identifier when present Added software_titles unique index idx_unique_sw_titles Jan 29, 2025
@ksykulev ksykulev force-pushed the 25235-software-titles-uniqueness branch from 05b41a5 to 581bfe9 Compare January 29, 2025 22:33
@gillespi314
Copy link
Contributor

I'm guessing the fkey was left off for performance reasons,

Yes, I think that was the reason.

but for sanity we should probably clean those up in ReconcileSoftwareTitles before the "clean up orphaned software titles" step, unless I'm missing something.

By cleanup do you mean update the software table to set software.title_id = NULL where the id doesn't exist in the software_titles table?

@iansltx
Copy link
Member

iansltx commented Feb 3, 2025

@gillespi314 yep, that's that I mean. Sounds like we're good to do that.

This allows software with different names but the same bundle identifier
to be grouped under the same title. It also allows for software with the
same name but different bundle identifiers to be under two separate
titles.
Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

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

Code LGTM. Good to merge once CI passes.

@ksykulev ksykulev merged commit 1b02fbb into main Feb 3, 2025
32 checks passed
@ksykulev ksykulev deleted the 25235-software-titles-uniqueness branch February 3, 2025 19:23
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.

3 participants