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

Unable to properly encapsulate ConfigService from process.env in tests #245

Open
manu-unter opened this issue Jun 2, 2020 · 18 comments
Open
Labels
enhancement New feature or request

Comments

@manu-unter
Copy link

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
@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Jun 19, 2020

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 kamilmysliwiec added the enhancement New feature or request label Jun 19, 2020
@manu-unter
Copy link
Author

manu-unter commented Jun 25, 2020

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

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue?

@Nosfistis
Copy link

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?

@AckerApple
Copy link

AckerApple commented Sep 7, 2021

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,
})

@ryanmr
Copy link

ryanmr commented Oct 1, 2021

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.

mahdavipanah added a commit to mahdavipanah/nestjs-config that referenced this issue Feb 28, 2022
…'ignoreEnvVars' is 'true'

Closes nestjs#245
BREAKING CHANGE: changes the priority of variables loaded from different sources.
@Toilal
Copy link
Contributor

Toilal commented Mar 17, 2022

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.

@technoknol
Copy link

@Toilal you're life saver!

@adrianwix
Copy link

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

@es-lynn
Copy link

es-lynn commented Sep 14, 2022

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.

@jenoosia
Copy link

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

@Thore1954
Copy link

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

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 7, 2023

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

@Thore1954
Copy link

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.

@danLDev
Copy link

danLDev commented May 9, 2023

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
  }
}

@davidmosna
Copy link

Any update on this?

@sergey-shablenko
Copy link

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

#1497

@vtgn
Copy link

vtgn commented Nov 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet