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

InsertAsync on Postgres returns the wrong identifier #1131

Open
monoculture opened this issue Feb 9, 2023 · 11 comments
Open

InsertAsync on Postgres returns the wrong identifier #1131

monoculture opened this issue Feb 9, 2023 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@monoculture
Copy link

monoculture commented Feb 9, 2023

Bug Description

I have a table in Postgres with the following structure:

image

which is then mapped to the following model:

image

since upgrading to verison 1.13.0 InsertAsync has started returning an empty GUID for the identifier:

image

It works fine if the account used to connect to the database is a member of the super user role or is the owner of the table but fails when I use a normal user account that is not the owner which makes me think it might have something to do with information schema permissions. The same code worked using the previous version of RepoDb.

Interestingly the synchronous version of Insert does not seem to be effected.

Additional information:

I believe RepoDb is executing the following SQL internally to get informaiton about the table:

SELECT    c.column_name ,          Cast((          CASE
                    WHEN c.column_name = tmp.column_name THEN 1                    ELSE 0          END) AS BOOLEAN) AS isprimary ,          CASE
                    WHEN c.is_identity = 'YES'                    OR        position('NEXTVAL' IN upper(c.column_default)) >= 1 THEN true
                    ELSE false
          END                            AS isidentity ,          cast(c.is_nullable AS boolean) AS isnullable ,          c.data_type                    AS datatype ,          CASE
                    WHEN c.column_default IS NOT NULL THEN true
                    ELSE false
          END AS hasdefaultvalue
FROM      information_schema.columns c
LEFT JOIN          (                 SELECT c.table_schema ,                        c.table_name ,                        c.column_name ,                        c.column_default
                 FROM   information_schema.table_constraints tc
                 JOIN   information_schema.constraint_column_usage AS ccu
                 using  (constraint_schema, constraint_name)                 JOIN   information_schema.columns AS c
                 ON     c.table_schema = tc.constraint_schema
                 AND    tc.table_name = c.table_name
                 AND    ccu.column_name = c.column_name
                 WHERE  tc.constraint_type = 'PRIMARY KEY' ) tmp
ON        tmp.table_schema = c.table_schema
AND       tmp.table_name = c.table_name
AND       tmp.column_name = c.column_name
WHERE     c.table_name = 'asset_type'AND       c.table_schema = 'public';`

when I execute this as a superuser I get:

MicrosoftTeams-image (11)

but when I execute the same query as a normal user i get the following results:

MicrosoftTeams-image (10)

@monoculture monoculture added the bug Something isn't working label Feb 9, 2023
@mikependon mikependon pinned this issue Feb 9, 2023
@mikependon
Copy link
Owner

It sounds to me that as per your post, this issue is not happening in an earlier version of the library. In the latest version, we have introduced the KeyColumnReturnBehavior that might had impact this case, though our Integration Tests has succeed. Would you be able to play around with that enumeration during startup?

@grofit
Copy link

grofit commented Feb 14, 2023

From what I can tell the key column return behaviour doesn't seem to effect the issue as its not correctly working out if its a primary key correctly so regardless of which one is set it always seems to fail in resolving what the primary key is from the DB side so just inserts an empty value for the data type.

I am not sure if this is a postgres only issue and while you do have integration tests it seems like you are using a single user which potentially has full permissions on each table so the problem wouldn't exhibit (from what I can tell).

I don't know enough about how postgres internally works, so not sure if our user is missing privileges that stops them getting the correct response for isprimary but all I can tell for sure is if you have a more locked down user it seems to fail regardless.

For us the locked down user has permissions like this:

GRANT ALL PRIVILEGES ON DATABASE <your-dbname> TO <your-user>;
GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA <your-schema> TO <your-user>;
GRANT DELETE, INSERT, SELECT, UPDATE ON <your-table> TO <your-user>;

@mikependon
Copy link
Owner

mikependon commented Feb 14, 2023

Hi Gents, first of all, thanks for reporting this issue. We have investigated this issue and ...

The same code worked using the previous version of RepoDb.

We failed to replicate your claims about this issue working with the old version of the library. Would you be able to share which version it is working? Unfortunately, our last release has major changes which limit us to recover the old code of the library. And since we cannot recover, we cannot also do replicate the issue codebase-wise.

We urged you use the latest version moving forward, of course, please help us resolve this issue by supplying us the necessary information to replicate or "how to best fix this".

On the other hand, @grofit.

I am not sure if this is a postgres only issue and while you do have integration tests it seems like you are using a single user which potentially has full permissions on each table so the problem wouldn't exhibit (from what I can tell).

We followed your steps and TBH, we could not replicate the issue you mentioned about super user and normal user. We both create SUPER and Non-SUPER user and limit the privilege of the other one, and we failed it there. (See the attached project)

There, in the attached project, we simply created a project and created the corresponding user and schema/table on our development environment and call the method in discussed.

RDBI1131.zip

In case you would like to test your claim and do not want to be blocked while we are solving this issue, would you be able to inject your own customized IDbHelper?

  • Ideally, you can download and paste the PostgreSqlDbHelper actual code into your project. Name it CustomizedPostgreSqlDbHelper.cs.
  • Then, you can override the GetCommandText() implementation to see if you can manage to return the right value in the IsPrimary column. This is because you have a better control on your own environment. (We would happy to ask you to PR it if you solve it on your own scenario)
  • Lastly, once you're done with your customized database helper object, you can inject in the library with the code below.
DbHelperMapper.Add<NpgsqlConnection>(new CustomizedPostgreSqlDbHelper(), true);

It must be done "AFTER" the initialization method below.

//GlobalConfiguration.Setup().UsePostgreSql(); // RepoDb.PostgreSql (v1.13.0)
PostgreSqlBootstrap.Initialize(); // RepoDb.PostgreSql (v1.1.5)

To end with, please be mindful that it has been stated on our limitation page that the following are limitations.

  • Auto-generated PK column
  • Computed columns
  • Composite keys

The rationale behind is because the library is "by designed" only return single value, and that single value is the identity column (auto-increment). It cannot accommodate the latest values generated by the DB/Table after the insertion/update/merging of the data towards it.

@mikependon
Copy link
Owner

In the latest version v1.13.0, if you use the following setup.

var options = new GlobalConfigurationOptions()
{
    KeyColumnReturnBehavior = KeyColumnReturnBehavior.Identity
};
GlobalConfiguration.Setup(options).UsePostgreSql();

Such behavior will behave like it was in the past. Setting the KeyColumnReturnBehavior to PrimaryKey or PrimaryKeyOrElseIdentity would signal to the library to prioritize returning the PrimaryKey over the IdentityKey, but again, the library limitation of not able to return the generated column is still prevailing.

We might consider this as known-issue or limitation reported issue for now, until we have a better approach to general fix this issue.

@grofit
Copy link

grofit commented Feb 14, 2023

Thank you so much for looking into this, I think this may totally be our fault as we had not moved over to use the newer GlobalConfiguration.Setup(options).UsePostgreSql(); and were just using the prior way of setting up, so when im in the office tomorrow I will give that a try and confirm back, as that would also align with why the previous version worked as it was correctly initializing :D

If that is the problem is there any way to indicate to devs that the old approach is obsolete or even remove it entirely if that is no longer the way to setup (PostgreSqlBootstrap.Initialize())

@mikependon
Copy link
Owner

In the latest version, that class and also to the other extended libraries (i.e.: RepoDb.SqlServer, RepoDb.MySql, etc), they were all marked as obsolete already.

@grofit
Copy link

grofit commented Feb 15, 2023

Ok so thank you again for verifying this, I have moved the code over to use the newer approach but I still see same failures on insertion.

On my local system it works fine and correctly inserts the Ids (some Ids are using identity, some are manually set) but either way it all works perfectly fine using the older bootstrap approach or the newer global configuration one. However when I push the build out to another environment the same code fails there, so I fear there must be something different that I'm missing in terms of how things are setup on my local vs server setups.

I know locally im running a docker provisioned postgres v13, whereas on the server in question I think its using postgres 12 but the postgres on the server is setup externally to us, so not entirely sure what is different, will see if I can find some info, or maybe @monoculture has some more information from their investigations.

@grofit
Copy link

grofit commented Feb 15, 2023

Upon conferring it looks like it may be down to the "table owner" so when you did your test was one user the table owner and the other user was not? as that seems to be the deciding factor (from what we can tell) is going to indicate if it gets a true or a false for the isprimary. (i.e if your user is the table owner it should get true if it is not it seems to get false)

@mikependon
Copy link
Owner

@grofit - for you to not get blocked and have a controllability, can you able to override this for now until we rectified the problem? As I stated in the above comment, you can follow the step below

  • Ideally, you can download and paste the PostgreSqlDbHelper actual code into your project. Name it CustomizedPostgreSqlDbHelper.cs.
  • Then, you can override the GetCommandText() implementation to see if you can manage to return the right value in the IsPrimary column. This is because you have a better control on your own environment. (We would happy to ask you to PR it if you solve it on your own scenario)
  • Lastly, once you're done with your customized database helper object, you can inject in the library with the code below.

If you did this, we are happy to get the code from you on how did you solve this 2 both user. Looking forward

@grofit
Copy link

grofit commented Feb 15, 2023

Hey, sorry for delay. We had a chat and for the moment to unblock us we have just made the current user the table owner and it has gone away, which we did via following query per table:

ALTER TABLE <your_table> OWNER TO <your_user>;

This has stopped the issue happening on the environment so for reproduction on your side you can probably just make another user the table owner and it should happen. As I said before I don't know enough around the innards of Postgres to be able to ascertain why this is an issue, as maybe other DBs allow multiple table owners or different ways of handling it, but either way we have worked around it for now and at least we know the root cause.

@fenilrathod
Copy link

fenilrathod commented Feb 15, 2023

@mikependon I've also faced the same issue. As @grofit said, it is related to the user's table/column ownership only.

The issue is occurring, when using a DB user (who is not a DB Owner) having only read/write rights on specific tables. This is because of information_schema.column view. It shows only those columns that the current user has access to (by way of being the owner or having some privilege). Documentation

In the end, we directly use the user who was DB Owner to resolve the issue as we didn't have much time to investigate further.

@mikependon mikependon unpinned this issue Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants