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

fix: issue #5613 #5614

Closed
wants to merge 3 commits into from
Closed

fix: issue #5613 #5614

wants to merge 3 commits into from

Conversation

krazibit
Copy link

@krazibit krazibit commented Mar 4, 2020

No description provided.

@ohjeyong
Copy link
Contributor

ohjeyong commented Mar 5, 2020

This PR cause same error as you said on #5613 on my project which means my PR(#5525) breaks yours and this PR breaks mine. I found the problem.

In PostgresDriver#L606,

buildTableName(tableName: string, schema?: string): string {
    return schema ? `${schema}.${tableName}` : tableName;
}

This is the problem.
Even though the default value of connection option schema is public, if schema option is set on connection option, then buildTableName returns public.some_table_name but if not, then buildTableName returns just some_table_name.

My project doesn't set schema option and your project does.

I think there are two solutions.

Solution 1. Merge this PR and change buildTableName function like below.

buildTableName(tableName: string, schema?: string): string {
    return schema ? `${schema}.${tableName}` : `public.${tableName}`;
}

Solution 2. Change this where statement like this.

WHERE (${tablesCondition})
    AND "column_name" = '${tableColumn.name}'
    AND (
        ("table_schema" || '.' || "table_name") = '${table.name}' OR
        "table_name" = '${table.name}'
    )`;

Solution 1 is much cleaner and better but buildTableName function is referenced a lot, so I don't know is it ok to change like this. @pleerock

Solution 2 is kind of dirty but it will not make any side effects.

@krazibit
Copy link
Author

krazibit commented Mar 9, 2020

Yeah, you are right @ohjeyong .
I'll add the second solution to the PR

@krazibit
Copy link
Author

krazibit commented Mar 9, 2020

Or even shorter as a schema variable is available in the method can do

"table_name" = '${table.name.replace(`${schema}.`)}'

@cknuteson-firstlook
Copy link

Looking forward to this getting merged. Had to rollback to 0.2.22 for my project

@ohjeyong
Copy link
Contributor

@krazibit Can you add the commit?

@krazibit
Copy link
Author

Sure, would add the commit in a bit

@krazibit
Copy link
Author

@ohjeyong, can you verify please? thanks

@ohjeyong
Copy link
Contributor

Thanks. LTGM. @pleerock Can you verify please?

@winterec
Copy link

winterec commented Apr 7, 2020

This patch fixes it for me

@pleerock
Copy link
Member

oh I already merged #5801 before I saw this PR. It looks like author tried to fix same issue, but a bit different way. Is merged approach is correct and I can close this PR? adding @igoraguiar

@krazibit
Copy link
Author

closing this as already fixed in PR-5801 as per @pleerock last comment

@krazibit krazibit closed this May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants