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

Mapping-by-code aggregates component mapping for reused data class #3623

Open
emwl opened this issue Nov 11, 2024 · 0 comments
Open

Mapping-by-code aggregates component mapping for reused data class #3623

emwl opened this issue Nov 11, 2024 · 0 comments

Comments

@emwl
Copy link

emwl commented Nov 11, 2024

We just ran into an issue where a reused component class got new properties, but only one of the mapping was updated. This worked fine for the updated location, but every other location now tries to load data from columns that don't exist.

Whether this is a useful class model or not is a discussion for another day, but under the assumption that a component mapping is considered "local", it shouldn't bleed into other mappings. If it were a regular class mapping, it would make sense to do that.
Originally, we just reused the class because it served the same purpose, but now after the change it seemed like we should use a distinct class for it. That's our current workaround on NHibernate 5.5.2 anyways, since we needed a way forward for the time being.

Our simplified code looks like this:

public class Entity1
{
	public Guid Id { get; set; }
	public string Name { get; set; }
	public IList<ComponentListEntry> Entries { get; set; } = new List<ComponentListEntry>();
}

public class Entity2
{
	public Guid Id { get; set; }
	public string Name { get; set; }
	public IList<ComponentListEntry> Entries { get; set; } = new List<ComponentListEntry>();
}

public class ComponentListEntry
{
	public string DummyString { get; set; }
	public string Property1 { get; set; }
	public string Property2 { get; set; }
}

mapper.Class<Entity1>(rc =>
{
	rc.Table("Entity1");
	rc.Id(x => x.Id, map => map.Generator(Generators.GuidComb));
	rc.Lazy(false);
	rc.Property(x => x.Name);

	rc.Bag(
		x => x.Entries,
		v =>
		{
			v.Table("Entity1Entry");
			v.Key(x => x.Column("Entity1Id"));
		},
		h => h.Component(cmp =>
		{
			cmp.Property(x => x.DummyString);
			cmp.Property(x => x.Property1);
		}));
});

mapper.Class<Entity2>(rc =>
{
	rc.Table("Entity2");
	rc.Id(x => x.Id, map => map.Generator(Generators.GuidComb));
	rc.Lazy(false);
	rc.Property(x => x.Name);

	rc.Bag(
		x => x.Entries,
		v =>
		{
			v.Table("Entity2Entry");
			v.Key(x => x.Column("Entity2Id"));
		},
		h => h.Component(cmp =>
		{
			cmp.Property(x => x.DummyString);
			cmp.Property(x => x.Property2);
		}));
});

Note that ComponentListEntry has three properties, but every usage location only maps two of them. The real code had two properties that got extended to eight, but it illustrates the same issue.

The main problem we had with reproducing this is that mapping-by-code produces tables with too many columns if you let it create them on-the-fly, so the test would work during the DDL stage and only fail later once you actually try to roundtrip data. The serialized mapping confirms this.
We generally create our tables ahead of time then write a mapping for them, we don't let NHibernate create them on-the-fly; so it failed trying to read columns that didn't exist.

I can see two possible outcomes for this:

  1. This is a bug in mapping-by-code, as components shouldn't bleed into other class maps.
  2. This isn't supposed to be allowed, and mapping-by-code should inform the developer that this mapping doesn't make sense.

It looks like option 1 to me at the moment, since XML mappings allow this; but I'm fine with either decision. The main goal should be preventing the developer from shooting themselves in the foot, and we clearly just did that by breaking something that we didn't expect to be affected by this.

Test case is over here in my fork: emwl@813451f

emwl added a commit to emwl/nhibernate-core that referenced this issue Nov 11, 2024
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

No branches or pull requests

1 participant