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

Support hyphen - and other special characters in variable names #2601

Merged
merged 21 commits into from Oct 1, 2020

Conversation

develohpanda
Copy link
Contributor

@develohpanda develohpanda commented Sep 11, 2020

Updating variable access logic to work with {{ regular }} (which is current behavior) as well as {{ _['base-url'] }}, which caters for dashes and special characters in the key.

Nunjucks templates support JS square bracket notation out of the box, but not as the first node. For example, {{ ['base-url'] }} is invalid (as it is in JS) while {{ variable['base-url'] }} is valid. For this reason, we have exposed the template context object (which is provided to Nunjucks), on the variable _, allowing you to access root keys that contain dashes or special characters, like {{ _['base-url'] }}.

The context object is exposed under _ in addition to the current implementation, so the old syntax (without _) will still work, when importing workspaces, or upgrading versions of Insomnia, for example.

I selected _ somewhat arbitrarily. If we would like to change that to something like _ctx, _env, $, or a different character, now is the best time to chime in. _ feels least invasive.

The autocomplete drop-down has been updated to always prefix an environment variable with _, to keep things consistent. It will also only use square bracket notation where necessary. If there are no special characters, regular dot notation will be used, now prefixed with _.

Old left, new right, both work
image

Old returns NaN, new returns the correct variable
image

Hiding the _. prefix or the bracket syntax from the user in the nice rendered tag view is doable, and shouldn't be too tricky, but it would be somewhat misleading. Open to thoughts on that, but I will not change in this PR, it will be changed in a future PR.

Additionally, NeDB has rules to prevent a key from starting with $ or contain a ., so I have updated the regex key check for the environment editor. In the current implementation, errors are presented in console, and the changes do not persist (semi-silent failure), if you try to save something without following these rules which the existing regex did not check for.

image

Screenshots

Environment
image

Autocomplete dropdown image
Tags image

Raw syntax
image

Not found errors
image

Variable editor modal image
Regex Checks image image image
Most importantly, this doesn't impact any existing tags image image image

TODO:

  • Extract reusable regex and hard-coded strings into constants
  • Ensure folder environment editor handles hyphen the expected way
  • Double check all regex related changes
  • Add unit tests where possible

To test:

  • Ensure export/import works
  • Check importers (postman)
  • Check openapi import
  • Ensure environment editor presents appropriate errors for invalid keys
  • Ensure folder environment editor presents appropriate errors for invalid keys

Fixes #1666, Fixes #456, Fixes #1960, Related to #2051, Fixes INS-118

@develohpanda develohpanda changed the title Support '-' in variable names Support hyphen - in variable names Sep 11, 2020
@rhukster
Copy link

👍 as a developer who works with Twig on a daily basis, I was going to suggest a custom filter to handle the dashes and it seems exactly the approach you have already gone with. I think your solution is solid. Retains template capabilities, UI mostly hides the filter so it does not impact usability. Really looking forward to this.

@gschier
Copy link
Contributor

gschier commented Sep 11, 2020

Another option I've thought of in the past is to expose the root environment under $ (or something similar) just before rendering, then use square bracket syntax like {{ $['foo']['bar-haha+blah woo!']['baz'] }} instead of dot notation. You wouldn't need an extra filter and it works out of the box with nested properties.

As @rhukster mentioned, though, the UI hides it all so it doesn't really matter what it looks like as long as the implementation is solid and it's not a breaking change 👍

@rhukster
Copy link

Another option I've thought of in the past is to expose the root environment under $ (or something similar) just before rendering, then use square bracket syntax like {{ $['foo']['bar-haha+blah woo!']['baz'] }} instead of dot notation. You wouldn't need an extra filter and it works out of the box with nested properties.

Yah in Twig the option is there to use _context['foo']['bar-haha+blah woo!']['baz'] in the same way, I just didn't realize how similar Nunjucks was. The whole _context and square brackets is slightly uglier though, but if it's hidden, it really it comes down to what fits best for Insomnia.

@develohpanda
Copy link
Contributor Author

develohpanda commented Sep 11, 2020

I will explore this option as well 👍

Either way, the template and the UI will need to be different, so if the feature set is identical between the two approaches I would favour the custom filter still, as it feels a bit clearer/easier to explain. Although context provided by a $ also feels good. Will ponder 🤔

@linear
Copy link

linear bot commented Sep 15, 2020

@develohpanda

This comment has been minimized.

@develohpanda

This comment has been minimized.

@develohpanda

This comment has been minimized.

@netlify
Copy link

netlify bot commented Oct 1, 2020

Deploy preview for insomnia-storybook ready!

Built with commit a9c827e

https://deploy-preview-2601--insomnia-storybook.netlify.app

Copy link
Contributor Author

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Some comments for reviewers.

@@ -276,28 +277,30 @@ export async function getRenderContext(
function getKeySource(subObject, inKey, inSource) {
// Add key to map if it's not root
if (inKey) {
keySource[inKey] = inSource;
keySource[templatingUtils.normalizeToDotAndBracketNotation(inKey)] = inSource;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in this file relate to building the key source lookup, so we can know if data is coming from the base environment, a sub environment, or a folder. The notation needs to be the same as what the template tag itself contains, hence uses the forceBracket and normalize functions used by the key indexing logic.

const contextForKey = con.keyContext[cleanedStr];

// Only prefix the title with context, if context is found
title = contextForKey ? `{${contextForKey}}: ${title}` : title;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

contextForKey is root, or the sub environment, or the folder name, etc

it.each(['a', 'ab', 'a$', 'a$b', 'a-b', `a${name}b`, `${name}ab`])('%s should be valid', key => {
expect(ensureKeyIsValid(key)).toBe(null);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests to ensure a key cannot be _, or contain ., or start with $.

@@ -430,7 +431,7 @@ class App extends PureComponent {

async _handleGetRenderContext() {
const context = await this._fetchRenderContext();
const keys = getKeys(context);
const keys = getKeys(context, NUNJUCKS_TEMPLATE_GLOBAL_PROPERTY_NAME);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getKeys is recursive, and is defined in templating/utils.js. The second argument reflects the key prefix, as the algorithm traverses all nodes. We now start the traversal with _, so all keys are prefixed by _.

Comment on lines 71 to 80
// Check for invalid key names
// TODO: these only check root properties, not nested properties
if (value && value.object) {
for (const key of Object.keys(value.object)) {
if (!key.match(/^[a-zA-Z_$][0-9a-zA-Z_$]*$/)) {
warning = `"${key}" must only contain letters, numbers, and underscores`;
error = ensureKeyIsValid(key);
if (error) {
break;
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bug here in that both previously and now, only the root keys are checked. A recursive algorithm needs to be created to validate nested keys appropriately.

@develohpanda develohpanda marked this pull request as ready for review October 1, 2020 06:10
@develohpanda develohpanda changed the title Support hyphen - in variable names Support hyphen - and other special characters in variable names Oct 1, 2020
Copy link
Contributor

@sonicyeti sonicyeti left a comment

Choose a reason for hiding this comment

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

Ultra comprehensive, great work!

@koldoon
Copy link

koldoon commented Jul 1, 2021

IMO there is nothing wrong to simply remove _. prefix. In most cases it looks dirty and doesn't really help, so it should be removed everywhere when possible.

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