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

KVv2 json cursor jumps on "enter" #27569

Merged
merged 37 commits into from
Jul 10, 2024

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jun 21, 2024

  • enterprise tests pass

Description

Fixes Issue #27524. Took the scenic route to get to this solution. The heart of the problem is the KVv2 secret engine parses the secretData on every change to the json editor (detailed steps below). So when a user hits enter, the codemirror modifier is called and it compares a parsed json blob to a non parsed blob, determining they are different. That determination caused the cursor to jump to the beginning of the text editor—an annoying UX experience.

I tried a variety of solutions and landed on the last option:

  1. Only parse on save. This fails miserably, We do various actions that require the model.secretData to be updated before save. Things like toggling between kv form fields, revealing obscured values, and diff views.
  2. Tried parsing and saving the model.secretData on a new blur event I added to codemirror. This borks the same set of things as solution one. Changing the parsing event will always bork things because we need the kv form field data and json editor data to be in sync and they get out of sync if we save them at different events (kv form field saves on change). However, these approaches were worth a shot because it's how the other json editor instances all work and why none of them run into this issue.
  3. Tried the current solution once, realized it borked for all instances where comments (think policies) were used because a comment can't be parsed (you can use comments in the jsonEditor when the mode is "ruby" otherwise it defaults to Javascript which does not allow them). Came back to this solution after Claire also landed on this separately. I then made it work by adding in a try / catch and using the stringify helper to keep the shape of the object.

Details for reference

codemirrors .setValue—see here. Granted, we're on an older version, so here's the setValue we're actually using (I've tested the updated codemirror, it doesn't fix this).

Code walk through of why it wasn't working before:

  1. When someone is using or toggling to the JSON editor for KVv2 when "creating" or "creating a new version" of a secret we parse the json onChange.
  2. This triggers the codemirror-modifier modify function here.
  3. If you hit a value like "enter" (a whitespace value), then the namedArgs.content has been parsed, before the this._editor.getValue() has been. Meaning that for a whitespace value like "enter" we're comparing:
    {foo: 'beep'} vs { "foo": "beep" /n }

So this only happens on the KVv2 Secrets engine and on whitespace events. The other JsonEditor instances (policies, cubbyhole, etc) all parse the json value onSave like workflows.

TODO only if you're a HashiCorp employee

  • Labels: If this PR is the CE portion of an ENT change, and that ENT change is
    getting backported to N-2, use the new style backport/ent/x.x.x+ent labels
    instead of the old style backport/x.x.x labels.
  • Labels: If this PR is a CE only change, it can only be backported to N, so use
    the normal backport/x.x.x label (there should be only 1).
  • ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
    of a public function, even if that change is in a CE file, double check that
    applying the patch for this PR to the ENT repo and running tests doesn't
    break any tests. Sometimes ENT only tests rely on public functions in CE
    files.
  • Jira: If this change has an associated Jira, it's referenced either
    in the PR description, commit message, or branch name.
  • RFC: If this change has an associated RFC, please link it in the description.
  • ENT PR: If this change has an associated ENT PR, please link it in the
    description. Also, make sure the changelog is in this PR, not in your ENT PR.

@Monkeychip Monkeychip added ui regression Used to indicate possible regressions between versions backport/1.17.x labels Jun 21, 2024
@Monkeychip Monkeychip added this to the 1.17.1 milestone Jun 21, 2024
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jun 21, 2024
Copy link

github-actions bot commented Jun 21, 2024

CI Results:
All Go tests succeeded! ✅

@andaley andaley modified the milestones: 1.17.1, 1.17.2 Jun 24, 2024
@Monkeychip Monkeychip marked this pull request as ready for review June 27, 2024 21:12
@Monkeychip Monkeychip requested a review from a team as a code owner June 27, 2024 21:12
Copy link

github-actions bot commented Jun 27, 2024

Build Results:
All builds succeeded! ✅

@Monkeychip Monkeychip added backport/ent/1.15.x+ent Changes are backported to 1.15.x+ent backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent labels Jun 27, 2024
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Beautiful job, and thanks for the detailed description!

@Monkeychip Monkeychip merged commit 297f8cb into main Jul 10, 2024
31 checks passed
@Monkeychip Monkeychip deleted the ui/VAULT-28328/kv-cursor-jump-saga-cont branch July 10, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.15.x+ent Changes are backported to 1.15.x+ent backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent backport/1.17.x hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed regression Used to indicate possible regressions between versions ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants