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

Remove FlattenMaps due to issues with private and protected fields #13523

Open
2 tasks done
thaoula opened this issue Jun 18, 2023 · 9 comments
Open
2 tasks done

Remove FlattenMaps due to issues with private and protected fields #13523

thaoula opened this issue Jun 18, 2023 · 9 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them! typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@thaoula
Copy link

thaoula commented Jun 18, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

7.3.0

Node.js version

16

MongoDB server version

v18.16.0

Typescript version (if applicable)

5.1.3

Description

We have recently upgrade Mongoose from v7.0.3 -> v7.3.0 and noticed that calls to lean are now returning the Model type wrapped with FlattenMaps.

For example we we previously had a model typed in the following way -

// Model
const document: Model<Quote>;

// Example Repository Usage
public async getById(id: string): Promise<Quote> {
     const quote = await this.document .findById(id).lean().exec();

     // This used to be of type Quote but now FlattenMaps<Quote> and causes a Typescript error
     return quote;
}

Since v7.3.0 we have over 100 typescript errors complaining -

// Quote her is just an example it can be replaced with T
Type '(FlattenMaps<Quote> & Required<{ _id: string; }>)[]' is not assignable to type 'Quote[]' 

To enable us to build we to do the following -

// Model
const document: Model<Quote>;

// Example Repository Usage
public async getById(id: string): Promise<Quote> {
     const quote = await this.document .findById(id).lean<Quote>().exec();

     // This would be of type Quote now
     return quote;
}

Steps to Reproduce

Please see description above

Expected Behavior

The expected behaviour is that calls to to lean with be type to the T used by model unless overridden like I have done in my example above.

@vkarpov15
Copy link
Collaborator

What does your Quote type look like - do you use maps? This change was intentional because lean() means Mongoose won't convert maps. Your map types will still be POJOs.

@vkarpov15 vkarpov15 added this to the 7.3.2 milestone Jun 19, 2023
@vkarpov15 vkarpov15 added the needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue label Jun 19, 2023
@thaoula
Copy link
Author

thaoula commented Jun 19, 2023

Hi @vkarpov15 ,

As always, really appreciate your prompt replies.

What do you mean by maps in this context?

Our Quote is a complex object a gist of the Typescript interface definition is below -


export interface Quote extends Entity, QuotePricingOptions, AuditDetails {

    status?: QuoteSimpleStatus;
    quoteNumber: string;

    sections?: ItemSection[];
    theme?: Entity;
    terms?: string;
    links?: EntityLink[];
    date: Date;
    expiryDate: Date;

    note?: string;
    description: string;

    account?: Contact;
    company?: Entity;
    owner?: Entity;
    address?: Address;
    contact?: ContactSummary;
    customerQO?: string;

    totalBuy?: number;
    totalSellEx?: number;
    totalAdjustment?: number;
    totalTax?: number;
    totalSellInc?: number;

    prepaymentEx?: number;
    prepaymentTax?: number;
    prepaymentInc?: number;

    overridePrepaymentEx?: number;
    overridePrepaymentInc?: number;
    overridePrepaymentTax?: number;

    source?: Source;
    imported?: boolean;

    integrationId?: string;
    budget?: JobBudget;

    customerRef?: string;

    approvalDate?: Date;
    acceptedDate?: Date;
    sentDate?: Date;

    signature?: string;
    poAttachments?: Attachment[];
    poNumber?: string;

    approver?: Entity;
    acceptor?: Entity;

    policy?: QuotePolicy;
    version?: number;
}

We have a schema typed to the above interface and we having been using calls to .lean() or aggregations to get our data as POJO in that shape.

@vkarpov15
Copy link
Collaborator

JavaScript's Map class. In theory, FlattenMaps<T> should be exactly equal to T unless there's a Map somewhere in your interface. Can you please double check whether you have any Maps in your document interface?

Also, does TypeScript give you any more hints about why exactly Type '(FlattenMaps<Quote> & Required<{ _id: string; }>)[]' is not assignable to type 'Quote[]' ? Does TS indicate what properties are incompatible?

@thaoula
Copy link
Author

thaoula commented Jun 19, 2023

Hi @vkarpov15,

We do not use Javascript Maps in interfaces.

It seems that the issue may be related to a specific type call Lex. This is a LexoRank object (Jira style ordering) or string.
export type Lex = LexoRank | string;

The Mongoose schema is set to String. The LexoRank object will serialize to string and were not having any issues using it with Mongoose before.

Here is a full typescript error for Quote.

Type '(FlattenMaps<Quote> & Required<{ _id: string; }>)[]' is not assignable to type 'Quote[]'.
  Type 'FlattenMaps<Quote> & Required<{ _id: string; }>' is not assignable to type 'Quote'.
    The types of 'policy.prepayments.rules' are incompatible between these types.
      Type 'FlattenMaps<QuotePolicyPrepaymentRule>[]' is not assignable to type 'QuotePolicyPrepaymentRule[]'.
        Type 'FlattenMaps<QuotePolicyPrepaymentRule>' is not assignable to type 'QuotePolicyPrepaymentRule'.
          Types of property 'sortOrder' are incompatible.
            Type 'FlattenMaps<LexoRank>' is not assignable to type 'LexoRank'.ts

Here is another model called OpportunityView -

Type 'FlattenMaps<OpportunityView> & Required<{ _id: string; }>' is not assignable to type 'OpportunityView'.
  Types of property 'list' are incompatible.
    Type 'FlattenMaps<OpportunityViewListField>[]' is not assignable to type 'OpportunityViewListField[]'.
      Type 'FlattenMaps<OpportunityViewListField>' is not assignable to type 'OpportunityViewListField'.
        Types of property 'sortOrder' are incompatible.
          Type 'string | FlattenMaps<LexoRank>' is not assignable to type 'Lex'

I hope this helps
Regards,
Tarek

@vkarpov15
Copy link
Collaborator

What does the LexoRank type look like?

@thaoula
Copy link
Author

thaoula commented Jun 21, 2023

LexoRank is a class with methods and has on two class level fields. Below is a gist of the class and I have removed the body of most methods. We are expecting the toString value to persist to the database.

export class LexoRank {

    constructor(
        readonly value: string,
        readonly bucket = '0'
    ) {

        if (!LexoRank.isValidLexValue(value)) {
            throw `Invalid lex value "${value}"`;
        }

        if (!LexoRank.isValidLexBucket(bucket)) {
            throw `Invalid lex bucket "${bucket}"`;
        }

        this.value = value;
        this.bucket = bucket;

    }

    public toString() {
        return `${this.bucket}|${this.value}`;
    }

    public static first(): Lex;
    public static from(lex: Lex);
    public static nextBucket(bucket: string);
    public static prevBucket(bucket: string);
    public static between(lexBefore: Lex | null | undefined, lexAfter: Lex): LexoRank;
    public static between(lexBefore: Lex, lexAfter: Lex | null | undefined): LexoRank;
    public static between(lexBefore: Lex, lexAfter: Lex): LexoRank;
    public lessThan(lex: Lex) ;
    public increment();
    public decrement();

    private hasNonZeroLeadingChars() ;
    private append(str: string);
    private static parse(lex: string);
    private static isValidLexValue(value: string);
    private static isValidLexBucket(bucket: string);
    private static cleanTrailingZeros(str: string);
    private static incrementChar(char: String) ;
    private static decrementChar(char: String);
}


@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Jun 21, 2023
@vkarpov15
Copy link
Collaborator

I suspect the issue is the private fields. It doesn't look like TypeScript can iterate over private fields when creating a copy of a class. For example, the following script fails to compile with a Property 'append' is missing in type 'FlattenMaps<LexoRank>' but required in type 'LexoRank'. error:

import { FlattenMaps } from 'mongoose';

export class LexoRank {
    
    constructor(
        readonly value: string,
        readonly bucket = '0'
    ) { 

        this.value = value;
        this.bucket = bucket;

    }
    
    public toString() {
        return `${this.bucket}|${this.value}`;
    }

    private append(str: string) {}
}

type T1 = (FlattenMaps<LexoRank>) | string;

const X: T1 = new LexoRank('foo', '0');
const X2: LexoRank = X;

We're investigating this and looking for a workaround.

@vkarpov15
Copy link
Collaborator

vkarpov15 commented Jul 3, 2023

Realistically, there's no good way for Mongoose to support classes with private members with FlattenMaps. private and protected fields don't show up in keyof, so there's no way for Mongoose to omit fields from types that have private fields without losing the private fields.

We should consider removing FlattenMaps in our next backwards breaking release, and any other places where we use a recursive keyof pattern, in favor of encouraging people to not define document interfaces with maps. The idea that FlattenMaps just breaks when private or protected fields are involved in a hard-to-debug way is concerning.

@hasezoey @AbdelrahmanHafez what do you think? Keep FlattenMaps or remove?

@vkarpov15 vkarpov15 changed the title Calls to lean returns FlattenMaps<T> instead of type T specified on the Model. Remove FlattenMaps due to issues with private and protected fields Jul 3, 2023
@vkarpov15 vkarpov15 added discussion If you have any thoughts or comments on this issue, please share them! and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Jul 3, 2023
@vkarpov15 vkarpov15 modified the milestones: 7.3.2, 8.0 Jul 3, 2023
@hasezoey
Copy link
Collaborator

hasezoey commented Jul 4, 2023

what do you think? Keep FlattenMaps or remove?

from my understanding FlattenMaps is about mapping the ts type Map<key, val> to a Record<key, val>(plain object), which is representative of what gets returned, is this understanding correct?

also from what i can tell, typescript is correct, you cannot assign a object that looks like LexoRank to a field expecting a instance of a class and .lean does not return a instance of anything, it just returns a plain object (POJO)

so there is either we let FlattenMaps stay and have a more correct type1 and the user has to create a POJO interface, or we remove FlattenMaps and return a more inaccurate type2, but the user does not have to deal with it

  • *1: the current FlattenMaps as used in .lean does not remove functions / methods, which is inaccurate (see example code
  • *2: when FlattenMaps is removed the type would say that at that path it is actually a instance of the class, which it is not and would say that a somePath: Map is actually a Map, which it is not (it is a Record)

some example code

// NodeJS: 20.2.0
// MongoDB: 5.0 (Docker)
// Typescript 4.9.5
import * as mongoose from 'mongoose'; // mongoose@7.3.1

class SomeClass {
  public test!: string;

  public method() {}

  // private testprop!: string;
  // private pmethod() {}
}

interface Test {
  class: SomeClass;
}

interface TestPOJO {
  class: Pick<SomeClass, keyof SomeClass>;
}

// function is expected to return a "Test" interface with a instance of "SomeClass" at "class"
function test(): Test /* TestPOJO */ {
  // using undefined just as a placeholder for types
  const t = undefined as any as mongoose.FlattenMaps<Test>;

  t.class.method(); // type does not error, but is incorrect and does not work at runtime in .lean

  return t; // return works without protected or private fields or methods
}

uncommenting either private field will result in Property 'nameOfProperty' is missing in type 'FlattenMaps<SomeClass>' but required in type 'SomeClass'., though replacing Test return type with TestPOJO fixes it somewhat (but still methods are present, which is still incorrect)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them! typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants