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

Columns with name "Id" in JSon object causes System.ArgumentException in materialization #29380

Closed
jonnybee opened this issue Oct 17, 2022 · 28 comments · Fixed by #34466
Closed
Labels
area-json area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. type-bug
Milestone

Comments

@jonnybee
Copy link

Using EF Core 7.0.0-rc.2.22472.11 (also same in v7.0.0-rtm.22504.12)

When JSon child object has a column named Id (f.ex of type Int) the materialization will throw:
System.ArgumentException: 'Expression of type 'System.Object' cannot be used for constructor parameter of type 'System.Int32'

Make these modifications to NewInEFCore7 sample in EntityFramework.Docs to reproduce the bug:

public class Commit
{
    public Commit(int id, DateTime committedOn, string comment)
    {
        Id = id;
        CommittedOn = committedOn;
        Comment = comment;
    }

    public int Id {get; private set; }
    public DateTime CommittedOn { get; private set; }
    public string Comment { get; set; }
}

// Update BuildPostMetadata method to send in an id as constructor parameter

    for (var j = 0; j < random.Next(3); j++)
     {
           update.Commits.Add(new(j+1, DateTime.Today, $"Commit #{j + 1}"));
     }

When you run the sample it will now throw ArgumentException here:
image

If I change Id to a property with { get; set; } and no parameters to the CTOR this exception is thrown:
System.ArgumentException: 'Expression of type 'System.Object' cannot be used for assignment to type 'System.Int32''

@jonnybee jonnybee changed the title Columns with name "Id" in JSon object causes exception Columns with name "Id" in JSon object causes System.ArgumentException in materialization Oct 17, 2022
@ajcvickers
Copy link
Contributor

@maumar Looks like it could be a problem with constructor binding for JSON objects in the materializer.

@jonnybee
Copy link
Author

@maumar @ajcvickers Other properties of type int is deserialized OK - but when named Id will throw the ArgumentException. Could it be that Id is considered Key by convention for that type?

@ajcvickers
Copy link
Contributor

@jonnybee Yes, I suspect that it is something to do with that.

@maumar
Copy link
Contributor

maumar commented Oct 17, 2022

owned types that are part of the collection have hard-coded ordinal key named "Id" of type int. Related issue: #28594

@maumar
Copy link
Contributor

maumar commented Oct 17, 2022

we should at least throw in validation until flexible ordinal key is implemented

@ajcvickers
Copy link
Contributor

ajcvickers commented Oct 20, 2022

Note from triage: we should investigate if this is a materialization bug separate from #28594.

Possible workaround if the entity doesn't need the property to be called Id but Id is required in the JSON:

modelBuilder.Entity<Customer>()
    .OwnsOne(customer => customer.Contact, b =>
    {
        b.ToJson();
        b.OwnsMany(e => e.PhoneNumbers, b =>
        {
            b.Property(e => e.Key).HasJsonPropertyName("Id");
        });
    });

@maumar
Copy link
Contributor

maumar commented Mar 31, 2023

the problem comes from the fact that the Id property is considered to be a key.
In case of JSON entity each property is either column access (in case of key - we project the key column and access value based on that column index) or JsonElement.TryGetProperty (in case of normal JSON property). In this case Id column is considered to be a key, so we try to access it based on index (which isn't there) and hence the bug.

There doesn't seem to be a separate materialization bug, apart from #28594.

@maumar maumar removed this from the 8.0.0 milestone Mar 31, 2023
@johnatanstanley
Copy link

Hi, I'm also facing the same (or very similar) problem when a property is like:

public int [CLASSNAME]Id {get; set; } // Like "CommitId" in the first example of this post

If I'm not mistaken, entity use this nomenclature to generate reference between classes, so may be is "reserved" too.

@ajcvickers ajcvickers added this to the 8.0.0 milestone Apr 6, 2023
@ajcvickers
Copy link
Contributor

Notes from triage:

@Tizioincognit0
Copy link

  • It should be possible to have a property called "Id" that is not the ordinal key.

Is there a way to have this feature on EF 7.0.8?

@ajcvickers ajcvickers added the punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. label Sep 6, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0, Backlog Sep 6, 2023
@thomas-darling
Copy link

thomas-darling commented Nov 21, 2023

According to the Npgsql docs, they've deprecated their support for .HasColumnType("jsonb"), which worked perfectly, in favor of .ToJson() - which won't work for us until this is fixed, as we just got blocked by #32360.

Please prioritize fixing this - it's been open for more than a year already...

@haslingerm
Copy link

I ran into this today as well and it took me quite some time to figure it out (found this issue only after having identified the root cause). If this can't be fixed soon I'd suggest adding a warning to the documentation at least.

@btecu
Copy link
Contributor

btecu commented Jan 14, 2024

Just ran into this issue and spent a couple of hours until I got to this ticket.
A warning would be useful but even better fixing the issue.

@oleg-varlamov
Copy link

oleg-varlamov commented Jan 31, 2024

I also encountered this problem today. Microsoft writes everywhere about good JSON support in the EF, but in fact we have many problems that prevent the use of JSON in real applications. We are waiting for a solution from the EFCore team.

@ajcvickers ajcvickers self-assigned this Aug 14, 2024
@ajcvickers ajcvickers added this to the 9.0.0 milestone Aug 14, 2024
ajcvickers added a commit that referenced this issue Aug 19, 2024
Fixes #29380

There are several aspects to #29380:
- Constructor binding (already fixed)
- Round-tripping the `Id` property value (addressed by this PR)
- Persisting key values in JSON collection documents (tracked by #28594)

I investigated persisting key values, but identity lookup requires key values in the materializer before we do the parsing of the document. This means persisted key values are not available without re-writing this part of the shaper, which we can't do for 9.

To fix the main issue (round-trip `Id`) this PR changes the way identity on owned JSON collection documents works. Instead of discovering and using the `Id` property, we treat it like any other property. We then create a shadow `__synthesizedOrdinal` property to act as the ordinal part of the key.

We need to discuss:
- Is this breaking for any scenario that was working before?
- Does this put us in a bad place for the future of owned types with explicit keys?
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 19, 2024
ajcvickers added a commit that referenced this issue Aug 20, 2024
Fixes #29380

There are several aspects to #29380:
- Constructor binding (already fixed)
- Round-tripping the `Id` property value (addressed by this PR)
- Persisting key values in JSON collection documents (tracked by #28594)

I investigated persisting key values, but identity lookup requires key values in the materializer before we do the parsing of the document. This means persisted key values are not available without re-writing this part of the shaper, which we can't do for 9.

To fix the main issue (round-trip `Id`) this PR changes the way identity on owned JSON collection documents works. Instead of discovering and using the `Id` property, we treat it like any other property. We then create a shadow `__synthesizedOrdinal` property to act as the ordinal part of the key.

We need to discuss:
- Is this breaking for any scenario that was working before?
- Does this put us in a bad place for the future of owned types with explicit keys?
ajcvickers added a commit that referenced this issue Aug 20, 2024
* Persist `Id` values into owned collection JSON documents

Fixes #29380

There are several aspects to #29380:
- Constructor binding (already fixed)
- Round-tripping the `Id` property value (addressed by this PR)
- Persisting key values in JSON collection documents (tracked by #28594)

I investigated persisting key values, but identity lookup requires key values in the materializer before we do the parsing of the document. This means persisted key values are not available without re-writing this part of the shaper, which we can't do for 9.

To fix the main issue (round-trip `Id`) this PR changes the way identity on owned JSON collection documents works. Instead of discovering and using the `Id` property, we treat it like any other property. We then create a shadow `__synthesizedOrdinal` property to act as the ordinal part of the key.

We need to discuss:
- Is this breaking for any scenario that was working before?
- Does this put us in a bad place for the future of owned types with explicit keys?

* Added validation of no explicit keys
ajcvickers added a commit that referenced this issue Aug 20, 2024
* Persist `Id` values into owned collection JSON documents

Fixes #29380

There are several aspects to #29380:
- Constructor binding (already fixed)
- Round-tripping the `Id` property value (addressed by this PR)
- Persisting key values in JSON collection documents (tracked by #28594)

I investigated persisting key values, but identity lookup requires key values in the materializer before we do the parsing of the document. This means persisted key values are not available without re-writing this part of the shaper, which we can't do for 9.

To fix the main issue (round-trip `Id`) this PR changes the way identity on owned JSON collection documents works. Instead of discovering and using the `Id` property, we treat it like any other property. We then create a shadow `__synthesizedOrdinal` property to act as the ordinal part of the key.

We need to discuss:
- Is this breaking for any scenario that was working before?
- Does this put us in a bad place for the future of owned types with explicit keys?

* Added validation of no explicit keys
@ajcvickers ajcvickers reopened this Aug 20, 2024
ajcvickers added a commit that referenced this issue Aug 21, 2024
* Persist `Id` values into owned collection JSON documents

Fixes #29380

There are several aspects to #29380:
- Constructor binding (already fixed)
- Round-tripping the `Id` property value (addressed by this PR)
- Persisting key values in JSON collection documents (tracked by #28594)

I investigated persisting key values, but identity lookup requires key values in the materializer before we do the parsing of the document. This means persisted key values are not available without re-writing this part of the shaper, which we can't do for 9.

To fix the main issue (round-trip `Id`) this PR changes the way identity on owned JSON collection documents works. Instead of discovering and using the `Id` property, we treat it like any other property. We then create a shadow `__synthesizedOrdinal` property to act as the ordinal part of the key.

We need to discuss:
- Is this breaking for any scenario that was working before?
- Does this put us in a bad place for the future of owned types with explicit keys?

* Added validation of no explicit keys
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-rc1 Aug 21, 2024
@roji
Copy link
Member

roji commented Aug 21, 2024

@ajcvickers didn't this go into rc2 as opposed to rc1 (the milestone)?

@ajcvickers ajcvickers modified the milestones: 9.0.0-rc1, 9.0.0-rc2 Aug 21, 2024
@andreyerick
Copy link

Thx for the example @sclarke81. For anyone else still having trouble: the workaround doesn't seem to work if you're using No-Tracking queries, it throws an exception "shadow properties cannot be preserved when attempting to SaveChanges".

@ajcvickers ajcvickers removed their assignment Aug 31, 2024
@GaetanVigner
Copy link

Hello, thx for the fix, when can we expect to see a RC2 Nuget ?

@roji
Copy link
Member

roji commented Sep 10, 2024

@GaetanVigner in around a month,

@roji roji modified the milestones: 9.0.0-rc2, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-json area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-8.0 Originally planned for the EF Core 8.0 (EF8) release, but moved out due to resource constraints. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.