Page MenuHomePhabricator

[identity] Use DB conversion helpers from comm-lib
ClosedPublic

Authored by bartek on Dec 22 2023, 5:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 11, 9:05 AM
Unknown Object (File)
Mon, Sep 9, 10:56 AM
Unknown Object (File)
Sat, Sep 7, 9:03 PM
Unknown Object (File)
Sat, Sep 7, 12:44 AM
Unknown Object (File)
Sep 3 2024, 11:42 PM
Unknown Object (File)
Aug 31 2024, 6:41 PM
Unknown Object (File)
Aug 26 2024, 1:29 AM
Unknown Object (File)
Aug 25 2024, 11:37 PM
Subscribers

Details

Summary

This finally resolves ENG-5476.

Used database conversion traits from comm-lib wherever possible. For some places, I introduced the deprecated attribute. This diff could grow too large.

Depends on D10447

Test Plan

Identity compiles and runs. Commtest passes, also played with it manually.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Dec 22 2023, 6:22 AM

In the future we might consider refactoring parse_{some_business_logic_name}_attribute() too, but it's not a direct purpose of this task, so I replaced only these "universal" functions

services/identity/src/database.rs
1697 ↗(On Diff #34976)

clippy fix

services/identity/src/database/device_list.rs
380–393 ↗(On Diff #34976)

Refactoring this one blew my mind 🤯

services/identity/src/error.rs
33–34 ↗(On Diff #34976)

This one is mainly used in code that is going to be removed anyway as a part of ENG-5842 so for now I deprecated it.

services/identity/src/database.rs
1352–1359 ↗(On Diff #34976)

Ahh I realized I can do even better

services/identity/src/database/device_list.rs
38–42 ↗(On Diff #34976)

This enum turns out to be unused

In the future we might consider refactoring parse_{some_business_logic_name}_attribute() too, but it's not a direct purpose of this task, so I replaced only these "universal" functions

Can we make a task for it? (or possibly even just do it in another diff, seems like it would be quick)

services/identity/src/database.rs
1148–1151 ↗(On Diff #34976)

Nit: does this work?

1574 ↗(On Diff #34976)

Why are we removing parse_string_attribute but not this one?

This revision is now accepted and ready to land.Jan 2 2024, 4:21 AM
This comment was removed by bartek.
services/identity/src/database.rs
1148–1151 ↗(On Diff #34976)

Yep, missed this, thanks

1574 ↗(On Diff #34976)

Still used in yet-to-be-removed code

In the future we might consider refactoring parse_{some_business_logic_name}_attribute() too, but it's not a direct purpose of this task, so I replaced only these "universal" functions

Can we make a task for it? (or possibly even just do it in another diff, seems like it would be quick)

Yeah I'll do that

Apply suggestion: simplify one more place