Loopback-next: context bug

[darkgray]

Description / Steps to reproduce / Feature proposal

Current Behavior

Expected Behavior

_See Reporting Issues for more tips on writing good issues_
Code shows error at this block:

@loopback/context/src/context.ts
constructor(_parent?: Context | string, name?: string) {
if (typeof _parent === 'string') {
name = _parent;
_parent = undefined;
}
this._parent = _parent;
this.name = name || uuidv1();
}

[raymondfeng]

@darkgray Can you elaborate what's wrong? An example/test to illustrate the issue is appreciated.

[darkgray]

Compilation error, assigning string to Context. And in certain conditions it can't find context.

[raymondfeng]

Please add some code here.

[darkgray]

Fix it with this:

  constructor(_parent?: Context | string, name?: string) {
    if (typeof _parent === 'string') {
      name = _parent;
      _parent = undefined;
    } else {
      this._parent = _parent;
      this.name = name || uuidv1();
    }
  }

[darkgray]

Add this to application.ts (for a lb4 generated API app):

let security = <SecuritySchemeObject>{
  type: 'http',
  name: 'JWT security',
  scheme: 'bearer',
  bearerFormat: 'JWT',
};
let specs = this.getSync(RestBindings.API_SPEC);
specs.components = <ComponentsObject>{};
specs.components.securitySchemes = { jwtAuth: security };

specs.security = <SecurityRequirementObject[]>[];
specs.security.push(<SecurityRequirementObject>{ 'jwtAuth': [] });

[raymondfeng]

@darkgray I'm confused. Are you saying our code context.ts does not compile? If not, what's your code that fails to use Context?

[raymondfeng]

Which version of TS do you use?

[darkgray]

latest version 1.4.1 of @loopback/context

Before asking a ton questions, can you just open that file in any node supported IDE? it will tell you that like of code is assigning a string to a Context.

[raymondfeng]

I'm using VSCode and it's happy.

[raymondfeng]

npm run build also passes - compilation is successful.

[raymondfeng]

Which IDE do you use and what's the version of TypeScript is it configured?

[darkgray]

Look at the bolded lines below... do you see the string assignment to Context?

export class Context {
/**

• Name of the context
  */
  readonly name: string;
  protected readonly registry: Map = new Map();
  protected _parent?: Context;

/**

• Create a new context
• @param _parent The optional parent context
  */
  constructor(_parent?: Context | string, name?: string) {
  if (typeof _parent === 'string') {
  name = _parent;
  _parent = undefined;
  }
  this._parent = _parent;
  this.name = name || uuidv1();
  }

[raymondfeng]

When it reaches to this._parent = _parent, TypeScript is smart enough to infer that _parent is either Context or undefined. If _parent is a string (typeof _parent === 'string'), it's reassigned to undefined.

[darkgray]

Yes, but after the if block, the string becomes context.

[raymondfeng]

Yes, but after the if block, the string becomes context.

I don't understand. After if (first arg is string), its value is assigned to name and _parent is undefined.

[darkgray]

Discuss it with someone, if you are still not understanding.

[raymondfeng]

@darkgray The Context constructor code is valid TypeScript and it passes the build. See https://travis-ci.org/strongloop/loopback-next. I'm not sure how we can help further unless you have screenshot or console error message to show the compilation issue.

[raymondfeng]

A few other maintainers confirmed there is no issue with the Context constructor. Closing as invalid. Feel free to reopen if you have proof.

[rmg]

I think strict types are the best thing that LoopBack gained by switching to TypeScript. If this isn't working, what else could be broken without anyone knowing?! That kind of uncertainty worries me so I took some time to put his under a microscope and really make sure it is working properly.

Analysis

I started with a simple manual analysis of the code.

There are 3 valid types for the _parent parameter and 2 valid types for the name parameter, so there's only 6 possible combinations. The code only contains a single conditional before the line in question and it is dependent only on the type of one of the parameters. That makes this really easy to exhaustively prove the correctness with a type table.

Table

| arg _parent type | arg name type | this._parent = _parent type |
|----|----|-----|
| undefined | undefined | undefined |
| undefined | string | undefined |
| Context | undefined | Context |
| Context | string | Context |
| string | undefined | undefined |
| string | string | undefined |

Code Analysis

constructor(_parent?: Context | string, name?: string) {
  if (typeof _parent === 'string') {
    name = _parent;
    _parent = undefined;
  }
  // if param _parent was undefined it is still undefined
  // if param _parent was a Context it is still a Context
  // if param _parent was a string it is now undefined
  // if param _parent was some other type the TypeScript compiler would reject it
  this._parent = _parent;
  this.name = name || uuidv1();
}

So now _I'm_ convinced that as long as TypeScript is properly enforcing the parameter types then there's no way that this._parent = parent would be a string.

What If?

But what if it _is_ a string? How could that happen?

TypeScript's Type Checking

I took a look through the TypeScript docs and history and found that support for this form of type narrowing was added in Microsoft/TypeScript#8010 over 2 years ago, so it's a well established feature of the language and compiler. That pull request even includes examples that show type flows very similar to what I showed above. It seems unlikely that anyone would be using a version of TypeScript that is more than 2 years old, but I suppose it is possible.

So if the version of TypeScript being used was released in the last 2 years, is there a way to bypass this form of type checking? There are certainly a lot of flags for controlling many aspects of the compiler. I looked through the list and couldn't find anything that looked relevant, though --skipLibCheck did give me an idea.

Bypassing Type Checking

If a library is written in JS and had a TS declaration file, the declaration file _could_ be ignored. Since LB4 is itself written in TS, though, that would mean running against a pre-compiled JS version of LB4 but without the generated declaration files. Since @loopback/context, like the other LB4 modules, are published to npmjs.org as pre-compiled JS+declaration+TS, it seems like it might be possible to import/require the @loopback/context package while ignoring the declaration file for it.

So I took a look at the generated JS:

constructor(_parent, name) {
    this.registry = new Map();
    if (typeof _parent === 'string') {
      name = _parent;
      _parent = undefined;
    }
    this._parent = _parent;
    this.name = name || uuid_1.v1();
}

It is more or less identical to the TypeScript version. The main difference is the type annotations are gone. The typeof _parent === 'string' check is still there, though, which will still enforce that this._parent will _never_ be a string no matter what parameter is given as _parent. If all the type checking is disabled as described above it would be possible to assign a number or some kind of object to this._parent, but never a string.

Conclusion

@darkgray at this point I'm 100% certain that this._parent = _parent will never be a string, but if you tried _really_ hard and disabled all the safety checks you _could_ make it some other type. If your tools are reporting otherwise then it is most likely a bug in your tools. If you let me know the versions of the tools you're using I could probably help you figure out where to report it.

[darkgray]

Good detailed analysis. But this bug manifests as context not found and results in http 500. Otherwise I won't open this issue.

This is exactly the reason I don't do js or typescript. Too many rookie bugs out there. Not worth chasing and convincing.

[rmg]

Good detailed analysis. But this bug manifests as context not found and results in http 500. Otherwise I won't open this issue.

This is the first mention of a 500 error and a missing context. If you are seeing those errors then you probably should have reported them instead, including the details that were previously requested.

There is a bug in the fix you proposed in https://github.com/strongloop/loopback-next/issues/2284#issuecomment-457407947 that could cause a missing context bug.

This is exactly the reason I don't do js or typescript. Too many rookie bugs out there. Not worth chasing and convincing.

I put a lot of time and effort into analyzing the type error you _actually_ reported and I don't appreciate the implications of these statements.