Page MenuHomePhabricator

[services-lib] Implement DDB Attr conversions, deprecate old methods
ClosedPublic

Authored by bartek on Aug 10 2023, 3:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 4, 2:42 PM
Unknown Object (File)
Sat, May 4, 2:42 PM
Unknown Object (File)
Sat, May 4, 2:42 PM
Unknown Object (File)
Sat, May 4, 1:12 PM
Unknown Object (File)
Sat, May 4, 1:12 PM
Unknown Object (File)
Sat, May 4, 1:12 PM
Unknown Object (File)
Sat, May 4, 1:11 PM
Unknown Object (File)
Sat, May 4, 1:03 PM
Subscribers

Details

Summary

Implements traits introduced in the previous diffs for basic types and marks legacy methods as deprecated.

Depends on D8789

Test Plan

Tested later in the stack

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.Aug 11 2023, 2:26 AM
bartek added a reviewer: varun.

Potential ideas: (both of them are kinda iffy so up to you!)

  • Merge datetime and timestamp parsing logic and use the attribute type (String/Number) to differentiate between the two
  • We could do something like this for numbers (the dummy trait is required so there are conflicting implementations for FromStr types)
trait NumberFromAttr: FromStr<Err = ParseIntError> {}

impl NumberFromAttr for u8 {}
// ...
impl NumberFromAttr for i64 {}

impl<T: NumberFromAttr> TryFromAttribute for T {
  fn try_from_attr(
    attribute_name: impl Into<String>,
    attribute: Option<AttributeValue>,
  ) -> Result<Self, DBItemError> {
    match &attribute {
      Some(AttributeValue::N(numeric_str)) => {
        parse_integer(attribute_name, numeric_str)
      }
      Some(_) => Err(DBItemError::new(
        attribute_name.into(),
        Value::AttributeValue(attribute),
        DBItemAttributeError::IncorrectType,
      )),
      None => Err(DBItemError::new(
        attribute_name.into(),
        Value::AttributeValue(attribute),
        DBItemAttributeError::Missing,
      )),
    }
  }
}
services/comm-services-lib/src/database.rs
147–164 ↗(On Diff #29833)

Shouldn't we also handle incorrect type case?

This revision is now accepted and ready to land.Aug 11 2023, 4:14 AM

Potential ideas: (both of them are kinda iffy so up to you!)

  • Merge datetime and timestamp parsing logic and use the attribute type (String/Number) to differentiate between the two
  • We could do something like this for numbers (the dummy trait is required so there are conflicting implementations for FromStr types)

Thanks for these ideas! For sure we need to do a better number support, but I'd like to design this carefully. I'll create a follow-up task for these

EDIT: https://linear.app/comm/issue/ENG-4658/improve-database-type-conversions-for-numbers

services/comm-services-lib/src/database.rs
147–164 ↗(On Diff #29833)

Yep, we can follow the same convention as the others

Make timestamp parsing follow conventions