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

Add json attribute to variants #188

Closed
4n70w4 opened this issue Nov 24, 2019 · 7 comments · Fixed by #685
Closed

Add json attribute to variants #188

4n70w4 opened this issue Nov 24, 2019 · 7 comments · Fixed by #685
Labels

Comments

@4n70w4
Copy link

4n70w4 commented Nov 24, 2019

Case: I have 10 000 (and increasing) variants for one flag. In my code i wan't manualy write code for each variant key. I want get variant contain json with variant data. Example variant json:

{
"color": "black",
"discount": 3,
"category_id": 24
}

And in my code write one code when handle json from variant instead if/else/switch for each variant.

@4n70w4
Copy link
Author

4n70w4 commented Nov 24, 2019

@markphelps
Copy link
Collaborator

@4n70w4 thanks for the feature request. I think I can see the value in this, wondering if you think this attachment field should support strictly JSON or any value (like a blob).

Reason being, I don't want to get into validating user input as JSON, but perhaps I dont need to. Whatever the user saves as the value could be returned as an attachment.

What are your thoughts?

@4n70w4
Copy link
Author

4n70w4 commented Nov 26, 2019

@markphelps yes you're right, may be do payload in any format.

@markphelps markphelps added this to the Pre 1.0.0 milestone Nov 29, 2019
@markphelps markphelps added help wanted Halp and removed thinking labels Jan 21, 2020
@markphelps markphelps removed their assignment Feb 1, 2020
@markphelps markphelps removed this from the Pre 1.0.0 milestone Feb 19, 2020
@markphelps markphelps added keep and removed planned labels Jan 30, 2021
@amayvs
Copy link
Contributor

amayvs commented Feb 3, 2022

Hi @markphelps, we are currently working on this feature and wanted to get some input about how the field should be represented in the UI.

As per the comments above, we've implemented attachment field as a bytearray, do we expect the UI to have a simple textarea where user can copy/paste data or does it need to do any encoding/decoding?

But we feel that the field would be better served as JSON, and if binary attachment is required, it can be base64 encoded and included to the JSON structure.

Additionally, if you have other general thoughts about this feature then do let us know. Thanks.

Draft PR: #685

@markphelps
Copy link
Collaborator

Thanks for taking this on and for the PR @amayvs ! I agree I think JSON would be better than storing binary.

Yes for the UI I was thinking we would have a textarea that is optional, and perhaps verify that it is proper JSON before submitting. We'd likely want to do that validation on the server side as well before accepting the JSON and perhaps put a limit to the number of bytes that can be stored?

@kevin-ip
Copy link
Contributor

kevin-ip commented Feb 4, 2022

Hi @markphelps, What would you recommend the default limit to the number of bytes?

I am planning to add a new config, DataConfig (default with 1024 KB), and updating the Validator.Validate() to take a context.Context. The ValidationUnaryInterceptor will set the ctx with dataConfig before invoking Validator.Validate(...). The method will then get the config from the context and compare against the length of the attachment. WDYT?

@markphelps
Copy link
Collaborator

markphelps commented Feb 5, 2022

@kevin-ip honestly I think we can just hardcode the max of the attachment field to be 10kb or something similar. Then if we get a request to make it configurable we could do so later.

10kb of json is quite a lot for a single field I think

I was thinking something like this which I guess does make sense to be in the variant validator

const MAX_VARIANT_ATTACHMENT_SIZE = 10000

bytes, err := json.Marshal(req.Attachment)
if err != nil { ... }

if len(bytes) > MAX_VARIANT_ATTACHMENT_SIZE {
  return errs.InvalidFieldError("attachment", "max json payload size is 10kb")
}

or something similar?

That way we don't have to modify the validator itself to pass in the context or add a new config property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants