atsign-foundation / at_client_sdk

The Dart implementation of atSDK used for implementing Atsign's technology into other software
https://pub.dev/publishers/atsign.org/packages
BSD 3-Clause "New" or "Revised" License
1.47k stars 31 forks source link

Keys are sent in lowercase regardless of what case they are created #743

Closed ralfeus closed 1 year ago

ralfeus commented 1 year ago

Describe the bug If I create the key that has UPPERCASE letters, they are replaced with lowercase upon storing

To Reproduce Steps to reproduce the behavior:

  1. Create and save the key with the code:

    final Metadata _defaultMetadata = Metadata()
    ..isEncrypted = true
    ..isPublic = false
    ..namespaceAware = true
    ..ttl = 60000
    ..ttr = -1;
    
    var atKey = AtKey()
      ..key = key
      ..sharedBy = client.getCurrentAtSign()
      ..sharedWith = recipient
      ..metadata = _defaultMetadata;
    Logger.root.finest(atKey.key);
    final result = await client.put(atKey, value);
    Logger.root.finest(await client.getKeys(sharedWith: member.atSign));
    return result;
    }
  2. Observe the output of a kind:
    2022-10-06 13:15:05.715148: FINEST: AKAtClient.send(): **memberInviteRequest**
    2022-10-06 13:15:05.756193: FINEST: AKAtClient.send(): [@baboon9blue:**memberinviterequest**.namespace@81bored7, @baboon9blue:shared_key@81bored7]

Expected behavior Key case remains intact

gkc commented 1 year ago

Hi @ralfeus - thanks very much for this!

Yep that is the intended behaviour right now, but it's not explicitly documented anywhere. It would be a lot of effort to allow mixed-case key names now, as there are assumptions about it throughout the codebase - but of course we could do it if there's a lot of demand. How important is this to you? How important do you think it might be in general for the atProtocol & atPlatform?

ralfeus commented 1 year ago

In general it seems to be a conflict with the naming convention of the Dart and Java. I would understand if primary language was Python where snake_case is used. But both Dart and Java adopt camelCase so it would be logical to follow same naming conventions in key names.

My use case is generating keys from an enum, which according to Dart's best practices has camel case elements. So in case of forcible lowercasing those keys have to either be transformed to snake case or similar or accept the fact that keys generated automatically from Dart identifiers (same applies to Java) would be human unreadable and will require additional transformation.

In any case even if there is a decision to have all keys lowercase there must be a clear communication back to the client - either exception in case of using the uppercase letters or returned key name as it's stored in secondary so the client is aware of result.

ksanty commented 1 year ago

Needs arch discussion

gkc commented 1 year ago

@ralfeus we discussed this and TL;DR we're sticking with lowercase.

We will not throw an exception is using uppercase letters as this is a breaking change, however we will ensure the code docs are very clear about it, and I believe we should log warnings when non-uppercase keys are being used

@srieteja FYI as I think you're working on this.

srieteja commented 1 year ago

Update from https://github.com/atsign-foundation/at_server/issues/964: We are currently enforcing toLowerCase in AtKey.fromString() and AtKey.toString(). Additionally, this will also be triggered from methods in AtClient where the conversion to lowercase will be logged to address this issue.

srieteja commented 1 year ago

Changes have been made in at_commons package concerning actively enforcing lowercase on AtKey. Once the at_commons package has been published and those changes have been consumed at_client, this ticket will be closed. The following PR tracks some changes relevant to this ticket https://github.com/atsign-foundation/at_client_sdk/pull/808.

murali-shris commented 1 year ago

@srieteja please update the latest status

srieteja commented 1 year ago

The implementation is ready to be merged. Since this change affects a wide range of things, it is being thoroughly tested. Will be closed as soon as the changes are ready to be published.

srieteja commented 1 year ago

AtClient v3.0.55 has changes that actively enforce lower case on an AtKey. And this conversion is logged when the key or namespace of an AtKey contains uppercase characters so that users are aware of this conversion.

Summary: