config icon indicating copy to clipboard operation
config copied to clipboard

Unable to properly encapsulate `ConfigService` from `process.env` in tests

Open manu-unter opened this issue 5 years ago • 18 comments

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

When testing different configurations for services in our nest.js application, we would like to separate the process.env of the actual test runtime from the simulated environment inside the test. I don't want the tested application to know that e.g. process.env.NODE_ENV is set to test in general, and I want to be able to change it into something else without globally mutating process.env. I tried using ConfigModule.forRoot({ ignoreEnvValues: true, ignoreEnvFile: true, load: [() => myTestConfig] }) for that purpose.

This doesn't suffice though because the evaluation order of different config sources inside ConfigService#get doesn't respect this configuration. It will still use this order (see source):

  1. validated configuration values set on initialization
  2. lazily evaluated process.env
  3. internalConfig - this is where myTestConfig ends up

Unfortunately, this results in the ConfigService still picking up process.env.NODE_ENV from the outer process before it would use myTestConfig.NODE_ENV.

There is a weird workaround: I can specify a validation schema, which will put myTestConfig into the internalConfig[VALIDATED_ENV_PROPNAME], which is evaluated first. I find this even more confusing.

Expected behavior

ConfigService#get respects the module configuration and doesn't fall back to process.env if ignoreEnvVars is set to true. I also think that the order of preference should be changed and the ConfigService should always check internalConfig before it falls back to process.env. I don't understand what's the motivation to do it the other way around.

Minimal reproduction of the problem with instructions

const module: TestingModule = await Test.createTestingModule({
    imports: [
        ConfigModule.forRoot({
            ignoreEnvVars: true,
            ignoreEnvFile: true,
            load: [() => ({ NODE_ENV: 'some_encapsulated_value' })],
        }),
    ],
}).compile();

const configService = module.get(ConfigService);
expect(configService.get('NODE_ENV')).toEqual('some_encapsulated_value');
// actual value: 'test' from the outer test runtime: `process.env.NODE_ENV`

What is the motivation / use case for changing the behavior?

This issue made me now mock the entire ConfigService and replace it with a different class entirely which I have under full control. I don't think this should be what nest.js requires us to do. After all, one of the big arguments for having a DI-powered configuration is that you can easily replace configuration with different values by creating a new ConfigService with a different underlying data source. Right now, we need to replace the whole ConfigService though.

Environment


Nest version: 7.1.0
@nestjs/config version: 0.5.0

manu-unter avatar Jun 02 '20 10:06 manu-unter

So basically, you would need a mechanism that allows disabling picking up the process.env variables in the ConfigService#get() method, is this correct? The ignoreEnvVars won't fit here since it only disables the validation of env variables (the name is somewhat confusing though).

kamilmysliwiec avatar Jun 19 '20 11:06 kamilmysliwiec

That or take the name of the config argument literally and couple it to this behavior

manu-unter avatar Jun 25 '20 20:06 manu-unter

Would you like to create a PR for this issue?

kamilmysliwiec avatar Aug 19 '20 13:08 kamilmysliwiec

So basically, you would need a mechanism that allows disabling picking up the process.env variables in the ConfigService#get() method, is this correct? The ignoreEnvVars won't fit here since it only disables the validation of env variables (the name is somewhat confusing though).

I think that the ignoreEnvVars should not apply just for validation. There is a separate validationOptions object for validation, where options like stripUnknown are. There is also the option of creating always valid entries in the validation schema, if validation needs to be skipped for some values. What would the use case be for ignoring process env entries just in validation, but including them in the final config?

Nosfistis avatar Sep 07 '20 11:09 Nosfistis

Came here under similar need and hoping for ignoreEnvVars to stop reading all process.env vars.

We pass all our configs with the types inferred into the ConfigModule but still get string "false" coming from ConfigService get.

In order for us to SOLVE for this we started using the following code:

ConfigModule.forRoot({
  load: [() => {
    const all = config.get() // nconf all variables

    // loop all vars and delete non exact-matches from process.env
    // We do this because @nestjs/config ALWAYS reads from process.env first so this causes it to read internally
    Object.entries(all).forEach(([name, value]) => {
      if (process.env[name] && process.env[name] !== value) {
        delete process.env[name]
      }
    })

    return all
  }],
  isGlobal: true,
})

AckerApple avatar Sep 07 '21 21:09 AckerApple

I tried using the ignoreEnvVars flag but it did not work for me. My use case was to temporarily override erroneous infrastructure injection at a code level.

Since process.env kept loading, I used the following to override it:

const config = () => {
  const settings: Record<string, any> = {
    /* ... various settings */
  };

  Object.entries(settings).forEach(([k, v]) => {
    process.env[k] = v;
  });

  return settings;
};

export default config;

This hack works for me, so I hoped to share.

ryanmr avatar Oct 01 '21 21:10 ryanmr

Here's my workaround to remove all variables defined in .env.test file from process.env when running tests only.

const envFilePath = process.env.NODE_ENV === 'test' ? '.env.test' : undefined

@Module({
  imports: [
    CleanEnvironmentModule.forPredicate(envFilePath, () => process.env.NODE_ENV === 'test'),
    ConfigModule.forRoot({
      expandVariables: true,
      envFilePath: envFilePath,
      ignoreEnvVars: process.env.NODE_ENV === 'test'
    })
  ]
})
export class AppModule {
}

CleanEnvironmentModule must be declared before ConfigModule.forRoot, sources for CleanEnvironmentModule are available here.

Toilal avatar Mar 17 '22 11:03 Toilal

@Toilal you're life saver!

technoknol avatar Apr 02 '22 10:04 technoknol

Shouldn't we update this module to actually do what is expected? "ignoreEnvVars" should ignore the environment variables and add "validateEnvVars" to do what the current "ignoreEnvVars" do.

This would be a breaking change though. But it should make the API easier to use in the future

adrianwix avatar Aug 25 '22 19:08 adrianwix

I have a similar problem where I inject JSON strings through .env, then parse it as JSON inside the config. Yet ConfigService still decides to load the stringified version from process.env instead.

I've managed to come up with 2 different workarounds to get ConfigService to stop returning the .env variable.


1. Load environment variables through the validate property instead of through the load property.

As mention in the first post, NestJS loads validated variables before it loads process.env variables. This takes advantage of that behaviour and injects the configurations through the validation function instead.

Before

ConfigModule.forRoot({
  load: [configuration]
  ...
})

After

ConfigModule.forRoot({
  validate: configuration
  ...
})

Note that this causes a a problem where configs have a key, but the value is undefined. ie. config = () => ({ port: undefined })

ConfigService ends up injecting port into process.env but sets it as "undefined" string. So it ends up as process.env.port = "undefined". This may cause bugs in your code as "undefined" string is evaluated as true.

So you have to remove undefined keys before passing it into the function. The final code ends up looking like this:

const config = () => ({ 
   port: undefined,
   debug: true,
   domain: 'http://localhost:3000'
})

function filterUndefinedKeys<T extends Record<any, any>>(obj: T): T {
  const objCopy = JSON.parse(JSON.stringify(obj))
  const undefinedKeys = Object.entries(objCopy).filter(([_key, value]) => {
    return value === undefined
  })
  undefinedKeys.forEach(([key, _value]) => {
    delete objCopy[key]
  })
  return objCopy
}

@Module({
  imports: [
    ConfigModule.forRoot({
      isGlobal: true,
      validate: () => removedUndefinedKeys(config())
    })
  ]
})

This gets NestJS to load your variables through the validate function, which takes precedence over environment variables.


2. Overwrite the code for ConfigService.get()

The other option I found was to simply just monkey patch the code for get() function and to remove the section where it reads from process.env.

// configuration.patch.ts
import { isUndefined } from '@nestjs/common/utils/shared.utils'
import { ConfigService } from '@nestjs/config'

ConfigService.prototype.get = function (
  propertyPath: any,
  defaultValueOrOptions?: any,
  options?: any
): any {
  const validatedEnvValue = this.getFromValidatedEnv(propertyPath)
  if (!isUndefined(validatedEnvValue)) {
    return validatedEnvValue
  }

  const defaultValue =
    this.isGetOptionsObject(defaultValueOrOptions) && !options
      ? undefined
      : defaultValueOrOptions

  /**
   * Comment out this section so it does not read from process.env
   */
  // const processEnvValue = this.getFromProcessEnv(propertyPath, defaultValue)
  // if (!isUndefined(processEnvValue)) {
  //   return processEnvValue
  // }

  const internalValue = this.getFromInternalConfig(propertyPath)
  if (!isUndefined(internalValue)) {
    return internalValue
  }

  return defaultValue
}

Then import it before your ConfigService is loaded. You may store it in the same file where you export your configuration.

// config/configuration.ts
import './configuration.patch'

export default () => ({
  port: parseInt(process.env.PORT, 10) || 3000,
  database: {
    host: process.env.DATABASE_HOST,
    port: parseInt(process.env.DATABASE_PORT, 10) || 5432
  }
});

Personally, both solutions feel like hacks to me. Option #1 feels like it could be potentially very confusing to future devs as this exploits undocumented behaviour.

Option #2 isn't ideal either and may break in future versions, but at least future devs will understand what you are trying to achieve. As such I've gone with #2.

es-lynn avatar Sep 14 '22 13:09 es-lynn

The related PR to this seems like it will fix a lot of headaches tied to the non-configurable priority order of process.env vars vs the configFactory. Would like to +1 this & see if we can get the PR merged

jenoosia avatar Sep 23 '22 10:09 jenoosia

The current evaluation order is illogical. process.env shouldn't take precedent over custom configuration files.

Thore1954 avatar Feb 07 '23 11:02 Thore1954

The current evaluation order is illogical. process.env shouldn't take precedent over custom configuration files.

If we ignore process.env then that would mean approaches like PORT=8080 nest startt --watch would no longer be valid and the PORT would always be read from .env files. What we currently do, use .env files to populate what is missing from process.env is also what dotenv does by default

jmcdo29 avatar Feb 07 '23 16:02 jmcdo29

What we currently do, use .env files to populate what is missing from process.env is also what dotenv does by default

Then it's reasonable to have an option to override process.env similar to dotenv.

Thore1954 avatar Feb 10 '23 12:02 Thore1954

Is there any update on this? I've seen a PR raised but it looks stale.

Experiencing the same issue but in the context of trying to transform some variables via the load option as below.

I've experimented with a couple of the solutions above, deleting or overriding process.env, however it appears that whatever I do, the original process.env is present during onApplicationBootstrap

RESTART_DEVICES_ON_STARTUP=true
const getEnv = () => ({
  RESTART_DEVICES_ON_STARTUP: stringToBoolean(process.env.RESTART_DEVICES_ON_STARTUP)
})

@Module({
  imports: [
     ConfigModule.forRoot({
        isGlobal: true,
        load: [getEnv]
     })
  ]
})
AppModule implements OnApplicationBootstrap {

  constructor(
    private configService: ConfigService<Env>,
  ) {
    
  }
  
  async onApplicationBootstrap(){
    const startDevices = this.configService.get('RESTART_DEVICES_ON_STARTUP'); 
    console.log(typeof startDevices) // String
  }
}

danLDev avatar May 09 '23 10:05 danLDev

Any update on this?

davidmosna avatar Jul 20 '23 15:07 davidmosna

could somebody explain why config service attempts to load values from process.env before reading from internal cache? why it sets all the variables back to process.env during initialization? is not it more consistent and performant to work with in memory cache rather than process.env why do you touch process.env at all when all of the flags to ignore env (.env files, process.env) are set to true this logic breaks type safety

https://github.com/nestjs/config/issues/1497

sergey-shablenko avatar Oct 19 '23 10:10 sergey-shablenko

Hi!

It would be a very useful feature that I'm searching for too.

For my case, I use this ConfigModuleOptions:

load: [my_micro_service_config],
my_validate,
cache: true,
ignoreEnvFile: true
  • my_micro_service_config is a function, building an object defining the only properties I want my micro-service(MS) can access by the ConfigService. These properties can be set by the process.env contents transformed or not, and can have the same names or not than the ones defined in process.env.
  • my_validate is a function which uses a Zod schema to validate and transform the properties names and values defined in my_micro_service_config object.

And I would like that the ConfigService must know only the validated and transformed properties defined in my_micro_service_config and not the global environment variables of the system too. Indeed, if in my custom config object I rename one of the properties and transform its value extracted from a global env variable, this global env variable will be still accessible by the ConfigService, and if a developer uses this bad global property instead of the modified one in my custom config, it could be problematic.

I would also find logical that the ConfigModule doesn't load by default the system global env vars, because they are already loaded and accessible by process.env, and generally, when we set a custom config, it is not to use the global one too: this should only be an option.

Regards.

vtgn avatar Nov 29 '23 18:11 vtgn