Skip to content

Conversation

@Sebazzz
Copy link
Contributor

@Sebazzz Sebazzz commented Apr 3, 2015

The documentation on IUserCollection.Instantiate already says that the collection needs to be uninitialized:

/// <summary>
/// Instantiate an uninitialized instance of the collection wrapper
/// </summary>
IPersistentCollection Instantiate(ISessionImplementor session, ICollectionPersister persister);

It would be nice to have a programmatic check that this assumption shall be true to prevent possible invalid or error-prone behaviour downstream.

@no149
Copy link

no149 commented Apr 4, 2015

Interestingly , I ran into this issue a short time ago and I in my case it was because a generic IList could not be cast to a non-generic IList. The reference code lies In PersistentGenericList.cs: 42. That is from the source code for Build 3.3.4.GA.
One straightforward solution is to inherit your custom collection from a non-generic appropriate collection interface. Another(as we chose) is to map your collection as a private field and create a wrapper property around it to expose it.

@Sebazzz
Copy link
Contributor Author

Sebazzz commented Apr 4, 2015

After some debugging I found out that this line, in the test, is the cause:

return new PersistentGenericSet<T>(session , new HashSet<T>(EqualityComparer<T>.Default));

Because the persistent collection is already given a wrapped collection, the persistent collection considers itself as initialized. Later on it is asked to check if it dirty, where it nullrefs because it hasn't got a snapshot (because it is considered initialized).

This is confusing API behavior, and probably a check needs to written that IUserCollectionType.Instantiate(ISessionImplementor, ICollectionPersister) doesn't return an initialized collection?

@Sebazzz
Copy link
Contributor Author

Sebazzz commented Apr 4, 2015

Added the proposed check to the pull request and amended the test for this check.

@hazzik

This comment has been minimized.

@Sebazzz

This comment has been minimized.

@Sebazzz

This comment has been minimized.

@hazzik
Copy link
Member

hazzik commented Jun 8, 2018

Took even longer to return here.

Can you please enable Allow edits from maintainers option on this PR?

The documentation on IUserCollection.Instantiate already says that the collection needs to be uninitialized:

/// <summary>
/// Instantiate an uninitialized instance of the collection wrapper
/// </summary>
IPersistentCollection Instantiate(ISessionImplementor session, ICollectionPersister persister);

@hazzik hazzik changed the title NH-3772 - Custom IUserCollectionType using PersistentSet<T> causes NullReferenceException in PersistentSet<T>.EqualsSnapshot Add check to ensure that IUserCollectionType.Instantiate returns uninitialized collection Jun 8, 2018
Copy link
Member

@hazzik hazzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code need to be aligned with the NHibernate's conventions.

I have a cleanup fix in place, I'll push it when option mentioned earlier be enabled.

@Sebazzz

This comment has been minimized.

…Instantiate returns an initialized collection. Add failing test for NH-3772.
hazzik
hazzik previously approved these changes Jun 8, 2018
Copy link
Member

@hazzik hazzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and code cleaned up

@hazzik hazzik force-pushed the NH-3772 branch 2 times, most recently from 8b63946 to 2f354bf Compare June 8, 2018 11:07
hazzik
hazzik previously approved these changes Jun 8, 2018
@hazzik hazzik requested a review from fredericDelaporte June 8, 2018 11:08

namespace NHibernate.Test.NHSpecificTest.NH3772
{
public class Fixture : TestCaseMappingByCode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lacking TestFixture attribute, causing it to not generate an async counterpart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

Copy link
Member

@hazzik hazzik Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not generate async code because action delegate is declared outside of Assert.That(...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but it still should have the attribute for consistency with other tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the attribute

hazzik
hazzik previously approved these changes Jun 8, 2018
@hazzik hazzik merged commit 0b66742 into nhibernate:master Jun 15, 2018
@hazzik hazzik added this to the 5.2 milestone Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants