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

fix: make namespace for local keys NOT mandatory #1135

Merged
merged 9 commits into from
Oct 26, 2023

Conversation

srieteja
Copy link
Contributor

@srieteja srieteja commented Oct 11, 2023

Closes #1075

- What I did

  • make namespace not mandatory for local keys

- How I did it

  • exclude local keys from namespace verification which takes place in AtClientValidation.validatePutRequest()

- How to verify it

  • unit tests added for public, private and shared keys asserting that namespace is required for these keys
  • unit test added for local keys asserting that namespace is not mandatory

- Description for the changelog

fix: make namespace for local keys NOT mandatory

@srieteja srieteja self-assigned this Oct 11, 2023
@gkc
Copy link
Contributor

gkc commented Oct 11, 2023

@srieteja why is this necessary?

@srieteja
Copy link
Contributor Author

@srieteja why is this necessary?

@gkc There was an issue a couple of months ago where the lastReceivedServerCommitId key was not being created if namespace is not set in at_client_preferences(no namespace is set while creating the key). We discussed this on the arch call and concluded that we should not enforce namespace validation on local keys to resolve this issue. I think I remember you were a part of the discussion.

more details at: #1075

@gkc
Copy link
Contributor

gkc commented Oct 11, 2023

Got it - thanks.

@murali-shris
Copy link
Member

make the version change in pubspec and AtClientConfig

@@ -97,14 +97,15 @@ class AtClientValidation {
}
// If key starts with 'cached:' (or) if the metadata of key has isCached set to true; it represent
// a cached key. Prevent client updating the cached key. Hence throw exception.
if (atKey.key!.startsWith('$CACHED:') ||
if (atKey.key!.startsWith('${AtConstants.cached}:') ||
(atKey.metadata != null && atKey.metadata!.isCached)) {
throw AtKeyException('Cannot create/update a cached key');
}
// If namespace is not set on key and in preferences, throw exception
if ((atKey.namespace == null || atKey.namespace!.isEmpty) &&
Copy link
Member

Choose a reason for hiding this comment

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

change this condition to
if (((atKey.namespace == null || atKey.namespace!.isEmpty &&) !atKey.isLocal) &&
(atClientPreference.namespace == null ||
atClientPreference.namespace!.isEmpty)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change made as part of 27600db

@srieteja srieteja requested a review from murali-shris October 13, 2023 10:36
@srieteja
Copy link
Contributor Author

srieteja commented Oct 13, 2023

make the version change in pubspec and AtClientConfig

The version has been updated to 3.0.66 in pubspec and at_client_config

@sitaram-kalluri sitaram-kalluri merged commit 5680849 into trunk Oct 26, 2023
7 checks passed
@sitaram-kalluri sitaram-kalluri deleted the namespace_localkey_not_mandatory branch October 26, 2023 05:26
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.

at_client expects internal keys to have namespace (when namespace is not provided through preferences)
4 participants