Insomnia v4 support for CLI

[MrSnix]

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

[develohpanda]

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

[MrSnix]

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

[nijikokun]

Great work, will simplify a lot of workflows 🎉

[develohpanda]

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

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

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]

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)

[develohpanda]

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

[MrSnix]

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]

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?

[develohpanda]

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

[vercel]

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

A member of the Team first needs to authorize it.

[abbasc52]

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

[develohpanda]

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.

[dimitropoulos]

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!

[MrSnix]

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.

[StevenReitsma]

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]

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!