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

Insomnia v4 support for CLI #3055

Merged
merged 24 commits into from
Sep 1, 2021
Merged

Insomnia v4 support for CLI #3055

merged 24 commits into from
Sep 1, 2021

Conversation

MrSnix
Copy link
Contributor

@MrSnix MrSnix commented Feb 6, 2021

TL;DR Contributor loses his sanity due to a crash generated by a typo (I presume)

Hi folks, as requested by PR #2666 I'm working on integrating Insomnia v4 support on CLI using a global option to generate an in-memory database that will be available for the lifecycle of the shell itself.

This is an early preview and I'm still working on it ⚠️ , I opened it because I feel like I need help 🌊 ⛵ 🌊 with an issue that is raised while delivering the Insomnia DB object to insomnia-send-request.

I'm using the following command to run the cli while developing from the dist folder:
node inso run test --fromFile ./../src/db/__fixtures__/insomnia-v4/Insomnia.json --verbose

As declared by the spec, I'm using the following Db model:

  export type Database = {|
    ApiSpec: Array<ApiSpec>,
    Environment: Array<Environment>,
    Request: Array<BaseModel>,
    RequestGroup: Array<BaseModel>,
    Workspace: Array<Workspace>,
    UnitTestSuite: Array<UnitTestSuite>,
    UnitTest: Array<UnitTest>,
  |};

Sadly, when reaching the following routine, even if I'm delivering the requested model, it dies:

  // Load lazily when needed, otherwise this require slows down the entire CLI.
  const { getSendRequestCallbackMemDb } = require('insomnia-send-request');
  const sendRequest = await getSendRequestCallbackMemDb(environment._id, db);

Console Output

› Data store configured from file at /Users/**/**/insomnia/packages/insomnia-inso/src/db/__fixtures__/insomnia-v4/Insomnia.json                                                     
› Prompt for document or test suite                                                                                                                                                                     
✔ Select a document or unit test suite · Swagger Petstore 1.0.0 - spc_c3cd1d
› Load api specification with identifier spc_c3cd1d from data store                                                                                                                                     
› Found 1.                                                                                                                                                                                              
› Load workspace with identifier wrk_91f4be0b00f24fe3b7b8fa54a4332ffc from data store                                                                                                                   
› Found 1.                                                                                                                                                                                              
› Load base environment for the workspace wrk_91f4be0b00f24fe3b7b8fa54a4332ffc from data store                                                                                                          
› Found 1.                                                                                                                                                                                              
› Prompt for environment                                                                                                                                                                                
✔ Select an environment · OpenAPI env - env_env_ee775e
› Load base environment for the workspace wrk_91f4be0b00f24fe3b7b8fa54a4332ffc from data store                                                                                                          
› Found 1.                                                                                                                                                                                              
› Load sub environment with identifier env_env_ee775e from data store                                                                                                                                   
› Found 1                   

 ERROR  Cannot read property 'find' of undefined                                                                                                                                                        

  at /Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:376:14
  at new Promise (<anonymous>)
  at Object.module.exports.database.find (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:375:10)
  at Object.module.exports.database.getWhere (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:397:31)
  at Object.module.exports.database.get (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:406:21)
  at module.exports.database.upsert (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:425:38)
  at Object.module.exports.database.batchModifyDocs (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:537:27)
  at processTicksAndRejections (internal/process/task_queues.js:97:5)
  at async getSendRequestCallbackMemDb (/Users/**/**/insomnia/packages/insomnia-send-request/dist/index.js:28951:3)
  at async runInsomniaTests (/Users/**/**/insomnia/packages/insomnia-inso/dist/index.js:597:23)

After some tests, I was able to figure out that the following parameter type is undefined when he reaches this function:

const find = database.find = async function (type, query = {}, sort = {
  created: 1
}) {
  if (db._empty) return _send('find', ...arguments);
  return new Promise((resolve, reject) => {
   // Basically, he access db['undefined'].find() and explodes
    db[type].find(query).sort(sort).exec(async (err, rawDocs) => {
      if (err) {
        return reject(err);
      }

After further investigations I found out the problem MAY comes from here:

const upsert = database.upsert = async function (doc, fromSync = false) {
  if (db._empty) return _send('upsert', ...arguments);
  const existingDoc = await database.get(doc.type, doc._id);

  if (existingDoc) {
    return database.update(doc, fromSync);
  } else {
    return database.insert(doc, fromSync);
  }
};

The function uses doc.type but the current declaration uses doc._type (with the underscore)...
I added the following lines of code on dist.js to check if I was right:

const upsert = database.upsert = async function (doc, fromSync = false) {
  if (db._empty) return _send('upsert', ...arguments);
  //TODO It was fixed here
  console.log('type: ' + doc.type)
  console.log('_type: ' + doc._type)
  const existingDoc = await database.get(doc.type, doc._id);
  [...]
};

Console Output:

type: undefined
_type: api_spec
type: undefined
_type: environment
type: undefined
_type: environment
type: undefined
_type: request
type: undefined
_type: request
type: undefined
_type: request
type: undefined
_type: workspace
type: undefined
_type: unit_test
type: undefined
_type: unit_test_suite

So, I wrote this little function just to check if the real fix was renaming to type

const rename: ApiSpec = (o, newKey, oldKey) => {
  delete Object.assign(o, { [newKey]: o[oldKey] })[oldKey];
  return o;
};

Sadly it crashes anyway, at the same spot for the same reasons apparently.

MrSnix, can you summarise? Things got complicated.
Well, yes.

Summary:

  1. I deliver the DB model (I assume correctly)
  2. database.upsert uses the BaseModel interface which declares type but on my object, I have _type
  3. database.upsert call database.find(type, db) with type = undefined
  4. database.find(type, db) dies because he tries to access db['undefined'].find()
  5. I wrote a little function to rename my object instance fields from _type to type
  6. I retry to see if now works...
  7. Crash anyway, same reason.

I have no clue what's happening there.

What should I do?
Any idea?

It closes #2666

Copy link
Contributor

@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.

The function uses doc.type but the current declaration uses doc._type (with the underscore)...

You're definitely on the right track with this.

Lets work with the UnitTest model for now. When exporting, models.unitTest.type is translated to EXPORT_TYPE_UNIT_TEST. It is then set to the property _type, and the type property is removed. (I'm not sure why this happens, but it does).

Therefore, when importing, we just need to do the reverse of this. Translate model._type = EXPORT_TYPE_UNIT_TEST to model.type = 'UnitTest' (this string value is defined in models.unitTest.type).

The import logic within the electron app (packages/insomnia-app) does something like that here then here.

The CLI (packages/insomnia-inso) cannot import the same logic, but it can also be a lot simpler.

If you do the following two things, you should be up and running again:

  1. Create a map to translate between the "export" type ('unit_test') and the "model" type ('UnitTest')
  2. Use that map to translate between _type and type in your extract method for each model, and delete the _type property

packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/cli.js Outdated Show resolved Hide resolved
@MrSnix
Copy link
Contributor Author

MrSnix commented Feb 9, 2021

I think I'm ready for another review. I don't know if I'm missing something else, please point it out as always.
I'll provide the new requested changes as quick as possible.

Thanks :D

@MrSnix MrSnix marked this pull request as ready for review February 9, 2021 23:12
@nijikokun
Copy link
Contributor

Great work, will simplify a lot of workflows 🎉

@develohpanda
Copy link
Contributor

@nijikokun we probably need to look at consolidating the --app-data-dir and --from-file flags into one; potentially keep the --app-data-dir flag but deprecate it in favor of --app-data-src which can be a directory (install directory) or a file (like the export file).

Do you have any thoughts about it at this stage, or we roll with introducing a new --from-file option?

@nijikokun
Copy link
Contributor

nijikokun commented Feb 12, 2021

@nijikokun we probably need to look at consolidating the --app-data-dir and --from-file flags into one; potentially keep the --app-data-dir flag but deprecate it in favor of --app-data-src which can be a directory (install directory) or a file (like the export file).

Do you have any thoughts about it at this stage, or we roll with introducing a new --from-file option?

There are a few future concerns I have that I'd like to walk through before making a breaking change to existing flags. Given that we want to support running multiple collections tests at once (entire space) for example what would be the best option.

Given the following options:

  • Individual v4.json file.
  • Directory of multiple v4.json files
  • Insomnia application data directory
  • Git Sync .insomnia directory

I will note, I do like the succinctness of --from-file. Looking at other tools they use very short and very direct terms such as --file and --dir, as close to these as possible is ideal. Looking at mocha, since I'm on it's page, they use --file <file|directory|glob> so perhaps a single option isn't as confusing.

So something like --src <file|directory|glob>?

Thoughts?

@develohpanda
Copy link
Contributor

develohpanda commented Feb 12, 2021

Yep that's what I'm thinking too. We can definitely work with a single option and try multiple db adapters (in priority) until one works, and I'm liking the succinctness of --src.

@MrSnix I'd say we replace --from-file <file> with --src <file|directory>. You might need to use fs.existsSync(src) && fs.lstatSync(src).isDirectory() to determine whether we're working with a file or directory, and load accordingly.

In addition we can still keep --app-data-dir around for backwards compatibility for now, but mark as deprecated in documentation. And anywhere we use --app-data-dir currently in the codebase, we also handle the --src flag.

We can introduce glob support in a future PR since that'll be a little more involved.

How does that sound?

@MrSnix
Copy link
Contributor Author

MrSnix commented Feb 13, 2021

Ok, ready for another review!

Changes:

  • Rewrote a lot of tests
  • Updated the way loadDb() works.
    It should be more clear and clean (at least, I hope so).
    I spent a lot of time figuring out how to make it short and simple,
  • Introduced some more checks on loadDb()
  • Changed --from-file to --src
  • Updated everything that was necessary
  • I'm taking the habit to make smaller commits which should be easier to review.

Please, let me know what you think and if I forgot something. (as always :P)

Copy link
Contributor

@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.

Large review for a large changeset, bear with me! 🤗 Good work with the insomnia adapter 👏🏽

The changes in packages/insomnia-inso/src/db/index.js in particular are quite tricky to review because of all the rules present and all of the changed unit tests. I don't think it quite matches the current behavior so I have reservations about accepting a large refactor to that file.

If it is possible, my preference would be to stick with the existing logic closely and try fit an extra if block in the logic to support insomnia export files. There may be some degree of code duplication and that's okay.

But before you embark on another refactor, were you facing issues with the existing logic that pushed you to look at making changes there? Just keen to understand the decision, in case I am missing something. 😄

packages/insomnia-inso/src/cli.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
@MrSnix
Copy link
Contributor Author

MrSnix commented Feb 16, 2021

Large review for a large changeset, bear with me! 🤗 Good work with the insomnia adapter 👏🏽

Smaller commits, longer changeset 😍
(I bear with you, you shouldn't bear with me xD, I write a lot of stuff, I increase your workload)

Thanks, appreciated.

The changes in packages/insomnia-inso/src/db/index.js in particular are quite tricky to review because of all the rules present and all of the changed unit tests. I don't think it quite matches the current behavior so I have reservations about accepting a large refactor to that file.

That's okay. I can revert to the original logic and work on it. 👍🏻 (no prob, really)

But before you embark on another refactor, were you facing issues with the existing logic that pushed you to look at making changes there? Just keen to understand the decision, in case I am missing something. 😄

Yes, sure.

  1. I usually refactor before updating or modifying something on the current code "hoping" to leave the code in a better state.
    (Even if, I honestly think, this refactor it's not so huge, was it?)
  2. I tried to improve the readability and reduce the complexity of the loadDb function.
  3. I felt like some stuff of the current implementation is quite tricky and a little bit (a tiny bit) weird:
  • Why is the CLI allowed to look for stuff inside a non-existing path? I know this is handled in some way but... still it's not that linear. For example, we know that a given path does not exist when we have tried loading all the adapters on it...Wat?
    It's not even that simple, we know that there is no data in that directory because all loaders failed so "officially" the directory may be valid but we don't know for sure if loaders failed due to data absence or path not existent... do you see my point?
  • Some adaptors have some IO checks before start working on data. Why these steps can't be done before and leave the adaptor alone with its code about being an adaptor rather than checking for data consistency.
  • Is it really necessary to check for db nullity in order to see if the next adapter is needed?
  • Why do we have 4/5 loggers which say the same stuff instead of looping on it?
  • Why can't we reduce some if with variables? (I know, the logic stays the same but it is cleaner)

I hope I clarified my point but I see, in some ways, this is a bit "legacy" so maybe it's better to not alter the current flow.

I want to thank you for the time you took to review my code, you so awesome (y), every change was made with the best intention but I'm okay if it is not the right way. Thanks again 😃 and again...and again...and again...

Let me know if you want definitely revert to the previous state because I saw some review comments left on it.

@MrSnix
Copy link
Contributor Author

MrSnix commented Feb 16, 2021

As requested:

Changes:

  • Tests have been reverted to their original state
  • loadDb() has been reverted to its original state (plus my code)
  • I addressed the changes requested by the code review
  • I simplified the multiple iterations in one single loop

Basic question:
I don't know exactly what I have to do when I have a directory of multiple v4.json files, should I merge everything in one big database? Is it correct?

Copy link
Contributor

@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.

Almost there! 🎉 A couple of observations

I don't know exactly what I have to do when I have a directory of multiple v4.json files, should I merge everything in one big database? Is it correct?

No need to handle that case with this PR (nor do I know the desirable answer to that at this stage, because there are nuances with each approach).

packages/insomnia-inso/src/cli.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/adapters/insomnia-adapter.js Outdated Show resolved Hide resolved
packages/insomnia-inso/src/db/index.js Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Mar 25, 2021

@MrSnix is attempting to deploy a commit to the Insomnia Team on Vercel.

A member of the Team first needs to authorize it.

@vercel vercel bot temporarily deployed to Preview March 25, 2021 13:37 Inactive
@abbasc52
Copy link

abbasc52 commented Jun 5, 2021

Any update on this? this PR seems to be pending for a long time

@develohpanda
Copy link
Contributor

Hey @abbasc52, sorry about the slow progress with this, it will be merged I just don't have a timeline right now 😞 will see if I can get to having a look at it again in the coming week.

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

re-tested everything and all looks good now. I re-ran linting as it has changed since this was last rebased and also made a very minor adjustment to an error message. thanks again @MrSnix! This is a big one!

@dimitropoulos dimitropoulos merged commit e8551bd into Kong:develop Sep 1, 2021
@MrSnix
Copy link
Contributor Author

MrSnix commented Sep 1, 2021

I'd like to thank @develohpanda and @dimitropoulos for the support and the time spent with me on reviewing this one:
you are always available, kind, and helpful.

I'm super glad to help this beautiful community.

@MrSnix MrSnix deleted the feature/cli-insomniaV4-support branch September 1, 2021 17:27
@StevenReitsma
Copy link

Is there going to be a new inso release soon that includes this PR? Building the project myself is easy but an npm release is even easier!

@wdawson
Copy link
Contributor

wdawson commented Sep 21, 2021

Hey, @StevenReitsma (and @MrSnix ). We're going to try to get a release out by the end of October. No promises but that's our target right now. Hopefully, we'll have a beta build before that too!

wongstein added a commit to Kong/insomnia-docs that referenced this pull request Jul 6, 2022
wongstein added a commit to Kong/insomnia-docs that referenced this pull request Jul 11, 2022
* Add changes made in this comment: Kong/insomnia#3055 (comment)

* Update docs/inso-cli/cli-command-reference/inso-export-spec.md

Co-authored-by: lena-larionova <54370747+lena-larionova@users.noreply.github.com>

* Update docs/inso-cli/cli-command-reference/inso-export-spec.md

Co-authored-by: lena-larionova <54370747+lena-larionova@users.noreply.github.com>

* Update options to Global Flags

* Add global flags to cli-command-reference

* Add global flags to all commands

Co-authored-by: lena-larionova <54370747+lena-larionova@users.noreply.github.com>
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.

Add insomnia V4 import support to Insomnia CLI
7 participants