App config in DynamoDB for Segment app #1698
Conversation
krzysztofzuraw
added
the
skip changeset
🦋 Changeset detectedLatest commit: f202748 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
krzysztofzuraw
changed the title
| export interface IAppConfigMetadataManager { | ||
| get(): Promise<AppConfig>; | ||
| set(config: AppConfig): Promise<void>; | ||
| export interface AppConfigMetadataManager { |
There was a problem hiding this comment.
metadata? metadata is specific to saleor metadata, we are opting out
There was a problem hiding this comment.
we can keep this interface but lets drop legacy naming. Why it's not just a ConfigRepository? Repository has normally built-in methods like "get" "getAll", "find" etc (CRUD).
You can extract DynamoDB specific part to separate DAO but I think it's an overkill.
IMO we can simplify:
DynamoConfigRepository implements ConfigRepository and MemoryConfigRepository for testing
| import { SegmentConfigEntityType, SegmentMainTable } from "./segment-main-table"; | ||
| import { ConfigRepository } from "./types"; | ||
|
|
||
| export class SegmentConfigRepository implements ConfigRepository { |
There was a problem hiding this comment.
I'd avoid prefixing with "Segment".
- You are in the context of segment app so you don't need this, of course it's related to the app
- In the context of Segment app, "segment" can be actual segment.io api/service which is confusing
ConfigRepository or AppConfigRepository will do
| }, | ||
| ) {} | ||
|
|
||
| async getEntry(args: { saleorApiUrl: string; appId: string; configKey: string }) { |
There was a problem hiding this comment.
please strong type the function return, this is the part I'd like to avoid infering
| config: schema({ | ||
| PK: string().key(), | ||
| SK: string().key(), | ||
| value: string(), |
There was a problem hiding this comment.
why value is string? Why don't you add normal fields? AFAIK there is some token field, so why it's not just a token?
There was a problem hiding this comment.
Question is: do we store every config value in its own attribute and encrypt it or rather we store encrypted JSON in one attribute (e.g configValue)?
Storing every config value in its own attribute means that we need to refactor a bit AppConfig class to allow serializing single config entry and encrypt / decrypt every config entry on its own - but I don't have strong opinion here
There was a problem hiding this comment.
- We should encrypt data that is sensitive, which is a token. Other fields don't have to be encrypted (like checkbox value)
- AppConfig during serialization can encrypt and during parisng decrypt itself
- AppConfig was serialized before because it was used as saleor metadata which is much simpler mechanism
Here I think we should
- Treat token as "one of the fields" in app config
- include encyrption/decryption in app config serialization / parsing. It can be in AppConfig but to avoid injecting db schema to domain, we should probably have separate class that will convert one object to another
We shouldn't drop native database features, having a single key is problematic, doesn't work with concurrency, hard to migrate etc
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
| setOrCreateConfig: protectedClientProcedure | ||
| .input(z.string().min(1)) |
There was a problem hiding this comment.
Maybe YAGNI but
- If we accept string, procedure probably should be setting only the token/key
- If we accept the config, we should accept the object. Can be a FormData, maybe serialized AppConfig?
There was a problem hiding this comment.
currently config is one segmentWriteKey so I think having setOrCreateSegmentKey will be better here as I didn't plan to refactor UI form (at least now)
| } | ||
|
|
||
| export class DynamoAppConfigManager implements AppConfigManager { | ||
| public readonly metadataKey = "app-config-v1"; |
There was a problem hiding this comment.
metadata key? I guess not anymore
There was a problem hiding this comment.
yes - it should be configKey instead 👍🏻
|
|
||
| constructor( | ||
| private deps: { | ||
| segmentConfigEntity: SegmentConfigEntityType; |
There was a problem hiding this comment.
does it really have to be injected? In general DI is good, Im just wondering if this is not a false decoupling. Isn't it by design coupled? When can it be replaced with something else?
There was a problem hiding this comment.
I will check but I bet it won't be replaced
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
Scope of the PR
AppConfig.Schemanot to havenullvalue any moreRelated issues
Checklist