Skip to content

"strictObjectIDCoercion": true on role-mapping.json#3198

Closed
luncht1me wants to merge 1 commit into
strongloop:masterfrom
luncht1me:luncht1me-patch-1
Closed

"strictObjectIDCoercion": true on role-mapping.json#3198
luncht1me wants to merge 1 commit into
strongloop:masterfrom
luncht1me:luncht1me-patch-1

Conversation

@luncht1me

Copy link
Copy Markdown

Description

How the json file is, loopback developers are currently unable to query RoleMappings by principalId, since most connectors (mongodb in my case) will overwrite the type of the principalId for the query to an ObjectId, which is incorrect behavior.

principalId is a string, as per this definition. So having the query cast to an ObjectId always returns no matches.

Related issues

How the json file is, loopback developers are currently unable to query RoleMappings by principalId, since most connectors (mongodb in my case) will overwrite the type of the principalId for the query to an ObjectId, which is incorrect behavior.

principalId is a string, as per this definition. So having the query cast to an ObjectId always returns no matches.
@slnode

slnode commented Feb 9, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode

slnode commented Feb 9, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@slnode

slnode commented Feb 9, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Feb 9, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Feb 9, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

@bajtos

bajtos commented Feb 15, 2017

Copy link
Copy Markdown
Member

@slnode ok to test

@bajtos

bajtos commented Feb 15, 2017

Copy link
Copy Markdown
Member

Hello @luncht1me, thank you for the contribution.

What are the consequences of your change for existing applications? I am concerned that this may be a breaking change.

We also require most changes to be accompanied with a test that fails before the change is made and passes afterwards. Can you add one please?

@bajtos bajtos self-assigned this Feb 15, 2017
@bajtos

bajtos commented Feb 16, 2017

Copy link
Copy Markdown
Member

@fabien @Amir-61 @jannyHou the flag "strictObjectIDCoercion" was added by loopbackio/loopback-connector-mongodb#297 which you authored/reviewed, could you please take a look at this related pull request too?

@fabien are you using loopback's built-in auth system? How do you fix object-id coercion for models like RoleMapping? Also, is there any reasonably easy way for LoopBack users to enable strictObjectIDCoercion flag in built-in models, for example via a boot script?

@fabien

fabien commented Feb 16, 2017

Copy link
Copy Markdown
Contributor

@bajtos this pull request looks fine - I have the following in a boot script right now:

RoleMapping.settings.strictObjectIDCoercion = true;

AFAIK this is the only built-in model that was affected by this option.

@bajtos

bajtos commented Feb 16, 2017

Copy link
Copy Markdown
Member

@fabien great! thank you for chiming in.

My main concern is about backward compatibility. If we enable strictObjectIDCoercion in LoopBack, can this change break existing applications after they upgrade to a newer LoopBack version?

@fabien

fabien commented Feb 16, 2017

Copy link
Copy Markdown
Contributor

@bajtos I think your concern is valid, as strictObjectIDCoercion will also dictate how ID's are written to mongo in the first place. In other words, it can only be enabled for 'fresh' projects, or after a db migration of legacy (string) ID's in existing applications.

Perhaps a migration script can be provided, though?

@bajtos

bajtos commented Feb 17, 2017

Copy link
Copy Markdown
Member

I see. In that case, let's enable this setting only for new projects created using lb/slc/apic tools - see strongloop/loopback-workspace#437.

I am closing this patch as rejected.

Perhaps a migration script can be provided, though?

That would be nice, indeed. I think this belongs to our documentation - instructions on how to enable strictObjectIDCoercion in existing projects. That's is out of scope of this patch though.

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.

5 participants