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

Support custom identity key for facts #336

Open
douglasg14b opened this issue Jun 16, 2023 · 4 comments
Open

Support custom identity key for facts #336

douglasg14b opened this issue Jun 16, 2023 · 4 comments

Comments

@douglasg14b
Copy link

douglasg14b commented Jun 16, 2023

It seems that the Update() workflow is broken when record class are utilized. Likely because of the value-type semantics of records, which mean when a record is changed, the HashCode changes, making any keys in say a dictionary change (In ways they are not supposed to), making the fact un-findable.

Records should be treated as immutable, however, the Update() method doesn't seem to support this as it expects the same object instance to be mutated, not a new object instance returned (ie. with the non-destructive mutation feature)

  • If I mutate a record with a different value in an Update() I receive a Facts for update do not exist exception.
  • If I use Update() the myRecord with { MyProp = newValue } syntax, I appear to enter an infinite loop of some sort
  • If I retract the fact, change the fact, and then insert the fact I end u in an infinite retract -> insert -> retract -> insert loop for the rule(s)

It also seems that because of the retraction of the fact in order to update it, it is not possible to make use of the [Repeatability] attribute 🤔

If there is already support for this, how? If not, what sort of outlook does such support have? Is this something I could help implement?

@douglasg14b
Copy link
Author

douglasg14b commented Jun 16, 2023

I'm browsing through the source of Update() UpdateAll() and TryUpdateAll(). I assume this is failing at the following line (returning null from the call):

var factWrapper = _workingMemory.GetFact(fact);

Which tried to retrieve this from a dictionary, with the key being the fact (The key that has now changed if the record was mutated). And thus it would probably fail to find the fact.

What side effects would there be from supporting an original and modified fact in a TryUpdateAll() overload? I imagine this would be more involved than just swapping out the value-like fact inside of _factMap?


Side note, this is a bit of a pain without SourceLink (wink-wink nudge-nudge), can't quite tell what is exactly happening, just speculating 😛

Edit: Ignore my SourceLink poking, saw your other post. I can always build from source and add a nuget source to the output dir

@douglasg14b douglasg14b changed the title [Feature Request] Support updates of record class facts or other value/value-semantic types [Feature Request] Support updates of record facts or other value/value-semantic types Jun 16, 2023
@snikolayev
Copy link
Member

@douglasg14b the thing to highlight here is that each fact is its own key in the working memory (i.e. it's looked up in a dictionary, like you pointed out), so regardless of something being a class or a record, the expectation is that the hash code won't change. In other words, facts are expected to have identity.
This is not a problem with records specifically, but immediately noticeable with mutable records due to value semantics.
While I personally believe mutable value objects (or mutable records with value semantics) are evil, I think there are other uses for this feature request, e.g. with classes where its equality comparison/hash code implementation are not suitable for use in the engine. So, I'm changing the title of this request to: allow inserting/updating facts with a custom identity key.
I think it's a reasonable feature request, so I'll look into it.

@snikolayev snikolayev changed the title [Feature Request] Support updates of record facts or other value/value-semantic types Support custom identity key for facts Jun 20, 2023
@douglasg14b
Copy link
Author

douglasg14b commented Jun 20, 2023

Thanks for taking a look!

I think this might be capturing two things, and the title captures 1.

  1. Classes with custom equality/hash code
    • Support here may be very difficult, it not outright impractical due to changing hashcodes if the object is mutated. Perhaps this is stated as a limitation in the same way that the base framework states these are limitations of dictionaries/hashsets...etc
  2. Immutable types (class || struct) with custom equality/hash code
    • It seems more reasonable for this to be a support option. Being able to update a fact by passing in a replacement. This also supports the idea that records are intended to be used this way, and even have syntax specially made just for this purpose. It also avoids touching the demons that are changing hash codes.

Personally support for #2 is more attractive, more reasonable, and doesn't cause any hardship to library users. Less about supporting custom identities, and more about supporting updating immutable facts without some the pitfalls of manually retracting & re-inserting them (ie. infinite loops).

Maybe I'm way off base, I'll let you comment.

This is something I'd be willing to put work in to help make happen, if that changes anything. I'd probably need some tech design & considerations from you.

@snikolayev
Copy link
Member

Maybe there is a better way to phrase this, I'm looking at this as a way to pass an identity key for a fact where existing identity is not suitable (case 1) or there is no identity (case 2).
I don't think this is going to be a major change and should mostly stay within the existing design. When facts are passed to the session for insert/update, first thing it does is it looks up the Fact container in the dictionary. This lookup is done using the fact object itself as a key. Just need a way to pass a custom identity key. Once Fact container is obtained, the rest of the library will remain unchanged.
If you get to it first, feel free to take a shot at implementation. I'll try to get to this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants