.findOne(undefined) returns first item in the database instead of undefined

[ct-reposit]

Issue type:

[ ] question
[x] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[ ] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[x ] postgres
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[x] latest
[ ] @next
[ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:

user.id = undefined;
User.findOne(user.id); // expect to see undefined / null but returns the first item in the table

[pleerock]

Looks like an issue. Contributions are welcomed!

[janmisek1]

Same problem with Mariadb

This:

User.findOne({ where: { id: null } })

works properly return undefined

[x2pdenjo]

Same here, why there is no fix?

[vlapo]

Is this real issue? I mean User.findOne(undefined); is same as User.findOne(); but what is correct behaviour of User.findOne();?

From jsdoc:

/**
 * Finds first entity that matches given conditions.
 */

But there are no conditions (undefined) and I think if there is no condition we have to return first entity like from User.findOne({});

Maybe change method signature and mark id or condition as required but it is really big change and it will be necessary sync at least all find* methods behaviour.

[janmisek1]

But there are no conditions (undefined) and I think if there is no condition we have to return first entity like from User.findOne({});

No, you except same behavior as SQL

SELECT * FROM users WHERE id = null LIMIT 1

return empty row not first entity.

[vlapo]

But there is function overloading:

findOne(id?: string|number|Date|ObjectID, options?: FindOneOptions<Entity>): Promise<Entity|undefined>;
findOne(options?: FindOneOptions<Entity>): Promise<Entity|undefined>;
findOne(conditions?: FindConditions<Entity>, options?: FindOneOptions<Entity>): Promise<Entity|undefined>;
findOne(optionsOrConditions?: string|number|Date|ObjectID|FindOneOptions<Entity>|FindConditions<Entity>, maybeOptions?: FindOneOptions<Entity>): Promise<Entity|undefined> {
        return this.manager.findOne(this.metadata.target, optionsOrConditions as any, maybeOptions);
}

You say undefined is for first overloaded function and represents id. I say it is possible that undefined is for any other overloaded method.

Your example with WHERE id = null can be right in case method name is findOneById() but I think findOne() is more about LIMIT 1 part.
But maybe I am wrong :-) lets wait to @pleerock or another contributors opinion.

[rustamwin]

typeorm/src/entity-manager/EntityManager.ts

Lines 580 to 621 in efb6353

	async findOne<Entity>(entityClass: ObjectType<Entity>|EntitySchema<Entity>|string, idOrOptionsOrConditions?: string|string[]|number|number[]|Date|Date[]|ObjectID|ObjectID[]|FindOneOptions<Entity>|FindConditions<Entity>, maybeOptions?: FindOneOptions<Entity>): Promise<Entity|undefined> {
	let findOptions: FindManyOptions<any>|FindOneOptions<any>|undefined = undefined;
	if (FindOptionsUtils.isFindOneOptions(idOrOptionsOrConditions)) {
	findOptions = idOrOptionsOrConditions;
	} else if (maybeOptions && FindOptionsUtils.isFindOneOptions(maybeOptions)) {
	findOptions = maybeOptions;
	}
	let options: ObjectLiteral|undefined = undefined;
	if (idOrOptionsOrConditions instanceof Object && !FindOptionsUtils.isFindOneOptions(idOrOptionsOrConditions))
	options = idOrOptionsOrConditions as ObjectLiteral;
	const metadata = this.connection.getMetadata(entityClass);
	let alias: string = metadata.name;
	if (findOptions && findOptions.join) {
	alias = findOptions.join.alias;
	} else if (maybeOptions && FindOptionsUtils.isFindOneOptions(maybeOptions) && maybeOptions.join) {
	alias = maybeOptions.join.alias;
	}
	const qb = this.createQueryBuilder(entityClass, alias);
	if (!findOptions || findOptions.loadEagerRelations !== false)
	FindOptionsUtils.joinEagerRelations(qb, qb.alias, qb.expressionMap.mainAlias!.metadata);
	findOptions = {
	...(findOptions || {}),
	take: 1,
	};
	FindOptionsUtils.applyOptionsToQueryBuilder(qb, findOptions);
	if (options) {
	qb.where(options);
	} else if (typeof idOrOptionsOrConditions === "string" || typeof idOrOptionsOrConditions === "number" || (idOrOptionsOrConditions as any) instanceof Date) {
	qb.andWhereInIds(metadata.ensureEntityIdMap(idOrOptionsOrConditions));
	}
	return qb.getOne();
	}

Probably need check idOrOptionsOrConditions like this

async findOne<Entity>(entityClass: ObjectType<Entity>|EntitySchema<Entity>|string|undefined, idOrOptionsOrConditions?: string|string[]|number|number[]|Date|Date[]|ObjectID|ObjectID[]|FindOneOptions<Entity>|FindConditions<Entity>, maybeOptions?: FindOneOptions<Entity>): Promise<Entity|undefined> { 
  if (!idOrOptionsOrConditions) {
    return undefined;
  }
....

[vlapo]

@rustamwin Did you mean idOrOptionsOrConditions instead of entityClass?

async findOne<Entity>(entityClass: ObjectType<Entity>|EntitySchema<Entity>|string|undefined, idOrOptionsOrConditions?: string|string[]|number|number[]|Date|Date[]|ObjectID|ObjectID[]|FindOneOptions<Entity>|FindConditions<Entity>, maybeOptions?: FindOneOptions<Entity>): Promise<Entity|undefined> { 
  if (! idOrOptionsOrConditions) {
    return undefined;
  }
...

This will cause that User.findOne() will also return undefined. I think this is incorrect behaviour of findOne. I would suppose User.findOne() will return one record without any conditions (if exists).

[rustamwin]

@vlapo Fixed, thnx 😄

[rustamwin]

Perhaps FindOptionsUtils.isFindOneOptions need to be improved @pleerock @vlapo

[vlapo]

From my point there is nothing to improve and findOne works as intended.

[pleerock]

It looks like we either need to left it as it is, either to enforce findOne to accept some real conditions. What do you think @Kononnable @havenchyk ?

[havenchyk]

@pleerock I personally would add a check if any value was passed or not

[vlapo]

@havenchyk but current method signatures allow this case User.findOne() and this check will throw error. Of course workaround is User.findOne({}). But in that case we should change method signature and require at least one parameter.

[havenchyk]

@vlapo why do you think it will throw an error?

just naive code:
...args => args.length > 0 ? args[0] === undefined && doSomething() : returnFirstItem()

of course, I see in code base it's much more complicated, but I don't the area for an error. Could you please explain, what I have missed?

[vlapo]

Sry I see you want to throw error in previous comment. My mistake :-)

So you vote for findOne() (without args) will return undefined. Not first one?

[havenchyk]

Seems I wasn't clear enough. Let me try once again.

If argument is passed and it's value is not undefined, handle this value.
If agrument is passed and it's value is undefined, return undefined/throw an exception
if agrument is not passed, return the first row.

[vlapo]

Understand now. I think it should work 👍

[pleerock]

If agrument is passed and it's value is undefined, return undefined/throw an exception

choose one. Return undefined or throw an exception?)

[Kononnable]

Sorry for being little late for the party.

No, you except same behavior as SQL
SELECT * FROM users WHERE id = null LIMIT 1

I completely disagree. Passing undefined shouldn't cause passing null to sql query. Null and undefined are completely different primitive values. Undefined should be ignored(optional parameters) or always return no records(without querying).

I think if we want to change current behavior we should throw an exception. If someone is deliberately passing value of undefined we can safely assume that he doesn't really know what he is doing and there is a human error somewhere. Returning undefined will just populate logical error to later called functions(app logic).

We can't search for something if we don't know what are we looking for - it means that there is a logical error while defining search criteria.

[mazyvan]

I'm having the same issue with findOneOrFail IMHO returning the first element when passing undefined as a parameter must not be the desired behavior. It's dangerous and no other ORM that I know does that

[matiasbeltramone]

The same issue for findOne and findOneOrFail, it's complete dangerous if you passing undefined as a parameter, must not be return the first element. :(

[Blockost]

Sorry for being little late for the party.

No, you except same behavior as SQL
SELECT * FROM users WHERE id = null LIMIT 1

I completely disagree. Passing undefined shouldn't cause passing null to sql query. Null and undefined are completely different primitive values. Undefined should be ignored(optional parameters) or always return no records(without querying).

@Kononnable Well, the same behaviour happens when calling findOneOrFail(null): first row is returned.

However, I agree that passing null or undefined does not make sense here and it probably means the code or the payload validation needs to be reviewed.

Having said that, sticking to what the translated SQL query would do is more intuitive for most people.

[mazyvan]

I think this could be fixed just adding a special configuration. So any user could decide by its own if he/she wants the findOne method to return the first item when passing an undefined value or not

[malimccalla]

This had me scratching my head for sooo long today! I wrongly assumed that findOne(userId) where userId is undefined would not return me a user. Is this a bug?

edit:
Have just fully read the above discussion and I can see it's not a bug. Will have to make sure to do extra type checks where findOne is used

[uPaymeiFixit]

This caused several hours of debugging for me today as well. Specifically with the method findOneOrFail (also affected). The name suggests the method will fail if it can't find a matching id in the database, like when the search target is null. This unexpected behavior seems to be causing a headache for many typeorm users.

I cannot find a scenario where a developer would expect to pass in undefined and receive the first row. While it's possible somebody is intentionally using the method this way, I would guess there are far more developers stumped by this.

[malimccalla]

If agrument is passed and it's value is undefined, return undefined/throw an exception

IMO this should be the default for findOne having it return undefined and findOneOrFail throw an exception.

edit:

Worth mentioning though that if you wish to avoid this you could just do .findOne({ where: { id } }) in which case undefined would be returned

[LaLaLaOpera]

Even It should have been our responsibility to check and make sure null or undefined parameter never go though query in a first place, but it is disappointing to see that the ORM still can't handle a simple exception like this.

[Chris4589]

con Equal(value) pude solucionar el error.

[issar13]

con Equal(value) pude solucionar el error.

doesn't this break other parts of an application?

[Chris4589]

no

[avilabiel]

TypeORM should be responsible to return the right data whenever there is a nullable or undefined value in findOne() function. This is weird and could be really dangerous depending on your application.

In my case, it returned a random value and we made our data dirty. It took a whole day to fix the messy data caused by this bug.

This is not fixed and should be reopened.

[issar13]

jst use the equal option on typeorm, fixes the issue

[avilabiel]

jst use the equal option on typeorm, fixes the issue

That does not make sense. If I'm finding one by id: null, the logical response would be bringing null, not the 1st resource/row.

[zmts]

v0.3.12 same issue ((

[TheRaLabs]

fix this asap

[DaviRJ]

The current behavior of this functionality is far from being even what appears in the TypeORM documentation. It needs to be fixed asap.

[Irungaray]

Spent a couple hours debugging this. The frontend was not sending the id on the body (as it was meant to), so .findOne was returning the first user, that was a super admin as @AbuOmar said.

Thankfully we caught it on a development enviroment, otherwise it could've been a serious security issue.

[HaiAlison]

in my case, I will do this:

import { IsNull } from 'typeorm';

const barn = await BarnsModel.findOneBy({
      id: dto.barn_id || IsNull()
    });

[brunomichalski]

in my case, I will do this:

import { IsNull } from 'typeorm';

const barn = await BarnsModel.findOneBy({
      id: dto.barn_id || IsNull()
    });

This works for me, but I don't think it should work this way

[HaiAlison]

in my case, I will do this:

import { IsNull } from 'typeorm';

const barn = await BarnsModel.findOneBy({
      id: dto.barn_id || IsNull()
    });

This works for me, but I don't think it should work this way

yes, it works when you ensure that the field does not have any null value

[1mehdifaraji]

I did solve the issue by using the Equal parameter from typeorm and using it this way

  import { Equal } from "typeorm";

  return await userRepo.findOne({
    where: { phone: phone ? Equal(phone) : Equal(bodyPhone) },
    relations: {
      addresses: true,
      favourites: true,
      cart: true,
    },
  });

[ofuochi]

.findOne({ where: { id } })

This has the same behaviour as returning the first record

[ezescigo]

So what would be the best practice to use findOne? or should we use other method?

[os-moussao]

So what would be the best practice to use findOne? or should we use other method?

Imo, using .createQueryBuilder() is the most convenient alternative to findOne() so far.

[mariopepe]

No way this must be a meme

[Alamonall]

LMAO. When is this will be fixed????

[landersonalmeida]

LMAO. When is this will be fixed????

Maybe in 2024 🤣

[issar13]

LMAO. When is this will be fixed????

Maybe in 2024 🤣

from 2018?.....no hope

[issar13]

everyone just use Equal()

[merthanmerter]

wow. thats a dangerous bug! and yes, it is a bug!

[issar13]

wow. thats a dangerous bug! and yes, it is a bug!

but wait there's more...apparently when you query findOne. It makes an sql query twice. turn logging on and see it in realtime.

[magyarb]

This is a huge security risk, and not the way it should work. Until PR #9487 is merged, I have created an eslint rule that warns about it.

rules: {
    'no-restricted-syntax': [
      'error',
      {
        selector:
          "ObjectExpression > .properties[key.name='where']:has(.value.properties > .value:not(CallExpression))",
        message:
          'Unsafe where condition, that can leak data. Use Equal() instead.'
      }
    ],
}

[merthanmerter]

This is a huge security risk, and not the way it should work. Until PR #9487 is merged, I have created an eslint rule that warns about it.

rules: {
    'no-restricted-syntax': [
      'error',
      {
        selector:
          "ObjectExpression > .properties[key.name='where']:has(.value.properties > .value:not(CallExpression))",
        message:
          'Unsafe where condition, that can leak data. Use Equal() instead.'
      }
    ],
}

Thank you a lot. To use it with nested object like the example, I modified as below.

where: { customer: { id: Equal(id) }, status: Equal(Status.ACTIVE) },

'no-restricted-syntax': [
      'error',
      {
        selector:
          "ObjectExpression > .properties[key.name='where'] > .value > .properties:not(:has(CallExpression)), ObjectExpression > .properties[key.name='where'] > .value > .properties > .value > .properties:not(:has(CallExpression))",
        message: 'Unsafe where condition, that can leak data. Use Equal() instead.',
      },
    ],

[YuriiBoikoOpn]

just leave here
so issue not just with 1 property in findOne
but also with another cases: findOne, findOneBy, findBy when there are many properties
example:
const patientId=120
const id= null

repository.findBy({id, patientId}) - will return not expected all raws where patientId=120 , but will skip id validate at all (like and id = undefined not exist )

more tested examples on local: