[identity] Use DB error types from comm-lib
ClosedPublic
Actions

Authored by bartek on Dec 22 2023, 5:48 AM.

Details

Reviewers

varun
michal

Commits

rCOMM81b299f2403e: [identity] Use DB error types from comm-lib

Summary

The DBItemError and DBItemAttributeError were defined both in Identity and comm-lib, however their signatures differ slightly.
Replaced the definitions in Identity with the ones from comm-lib, made slight modifications (added From derive) to make them compatible.
This isn't perfect and is kind of messy, but will be reworked later.

Depends on D10446

Test Plan

Identity compiles, tests pass.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Dec 22 2023, 5:48 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 22 2023, 5:48 AM

bartek added a child revision: D10448: [identity] Use DB conversion helpers from comm-lib.Dec 22 2023, 5:51 AM

The main issue here was that Identity attribute_value is of type Option<AttributeValue> while comm-lib is of enum type Value. The simplest way was to introduce From derive and use .into() where needed.

Perhaps we can do better but I didn't want this diff to grow too large (changing all usages in other services).

Harbormaster completed remote builds in B25321: Diff 34975.Dec 22 2023, 6:34 AM

If I understand the error type correctly, shouldn't we use Value::String in places where we directly create Some(AttributeValue::S(...)?

This revision is now accepted and ready to land.Jan 2 2024, 4:00 AM

In D10447#303397, @michal wrote:

If I understand the error type correctly, shouldn't we use Value::String in places where we directly create Some(AttributeValue::S(...)?

Yeah, good call. Not sure why the Value enum was introduced initially, but anyway your suggestion looks better now