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

load pids correctly #568

Merged
merged 2 commits into from
Sep 13, 2023
Merged

load pids correctly #568

merged 2 commits into from
Sep 13, 2023

Conversation

dpetran
Copy link
Contributor

@dpetran dpetran commented Sep 7, 2023

Fixes https://github.com/fluree/core/issues/24

This arose because we handle loading commits differently than we handle staging transactions. Commits are flattened versions of the txn, and ref objects don't have the type information of the referred node, so we were not correctly assigning pids to new ref flakes.

This adds a little bit of extra data to the assertion ref objects so that we can correctly generate a subject id.

If a predicate was assigned a pid during staging because of its type, that context is
missing during the loading of commit asserts. Because of this, queries using those
predicates (that now have sids instead of pids) return an invalid predicate error.
Since ref values in commit assertions only contain the ref id and not the additional
properties of the referred node that are required to determine if a pid or sid should be
generated, we need to enrich the raw flattened assertions with the additional properties
that we know about.

fixes fluree/core#24
@dpetran dpetran requested a review from a team September 7, 2023 21:37
@bplatz
Copy link
Contributor

bplatz commented Sep 8, 2023

This may solve a short-term issue, but there is a fundamental problem here still unaddressed and will require some more changes, perhaps we need to punt on them for the time being.

We will never know if something is a property unless it is (a) used as a property, (b) used as an object for a property whose values must be other properties, or (c) declared as a rdf:Property or a class that is a subclass of rdf:Property (such as owl:ObjectProperty or owl:DatatypeProperty).

There will always be circumstances where none of these conditions are known until some later point in time.

Ultimately we need to release the dependency that properties are specially assigned upon creation. This may be reworking some logic that relies on this dependency, or it may be keeping a new list/index that maintains things that were used as properties so we can add things to it post-creation for quick lookup.

Not sure, I only know this fix wouldn't have been necessary if we addressed the fundamental problem that still exists.

@dpetran
Copy link
Contributor Author

dpetran commented Sep 8, 2023

That's a good point, and I'm actually in the process of writing up a proposal for a general solution to this problem that I hope to share this afternoon. I did want to address the specific issue in the ticket, but I've been spending a lot of time fixing pid/sid bugs in the last month so I've had some good hammock time on the general problem and I think I've got a workable solution.

@dpetran
Copy link
Contributor Author

dpetran commented Sep 8, 2023

Copy link
Contributor

@cap10morgan cap10morgan left a comment

Choose a reason for hiding this comment

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

🥧

@dpetran dpetran merged commit 2b87706 into main Sep 13, 2023
5 checks passed
@dpetran dpetran deleted the fix/load-pids-correctly branch September 13, 2023 21:21
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.

3 participants