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

[POC] rails attribute for ancestor_ids #481

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Collaborator

@kbrock kbrock commented Jun 20, 2020

Rails has a concept of serializing attributes. They even allow you to define a custom serializer.

So the concept of ancestry and ancestor_ids are merged, and the delimiter basically goes away.

So from there, I merged them over to the ancestor_ids concept and saw if it was possible to rename fields based upon ancestor_ids. If this is possible, it may be possible to have multiple ancestors.

Dropping 4.2 to simplify this support.

elsif value.nil? || value.kind_of?(String)
value
else
byebug
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we want to handle this case a little differently

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol

@coveralls
Copy link

coveralls commented Jun 20, 2020

Coverage Status

Coverage decreased (-0.7%) to 97.418% when pulling ecdf320 on kbrock:attribute into b8f241f on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 97.487% when pulling 5f66c82 on kbrock:attribute into d7cfe4e on stefankroes:master.

extract the % string.
this is materialized path only, but it is nice to distinguish
between my child, and my descendants
ancestry is now an array

ancestor_ids are going to merge with ancestry
goal is to have all methods relative to ancestor_ids

if possible names will contain ancestor_ids

then, we may be able to configure this to have multiple parents
@kbrock kbrock changed the title rails attribute for ancestor_ids [POC] rails attribute for ancestor_ids Jan 21, 2021
@kbrock
Copy link
Collaborator Author

kbrock commented Jan 21, 2021

this turns out to not be trivial to rebase and bring forward. frustrating
have tried a few times and no dice.
especially frustrating since this really doesn't touch anything that changed that much.

the issue I have here is rails 5.1(?) and earlier don't handle the serialization that well - refusing to cache it and not properly detecting when the value has changed. Well, to be more precise, not detecting when the value has not changed. It also does not cache the casting so quite a few more objects are created.

I would prefer a way to make the attribute look more like a rails database parameter. The integration points are close but not quite there in rails proper. Requires just a little too much monkey patching.

i do have a modified version of activerecord-virtual_attributes that is more native. but again, it is close but still requires too many monkey patches.

@kbrock
Copy link
Collaborator Author

kbrock commented Apr 13, 2021

so it looks like there are a few things to do for this

Determine whether we want to expose ancestry as a string for compatibility, or we want to expose as an array, and use ancestry_before_type_cast to represent the serialized value. Or if we want to use ancestor_ids as the core concept and throw ancestry in there for legacy purposes.

  • get away from directly manipulating the serialized version of the ancestor_ids (aka ancestry field) - it is an implementation detail on how the tree of ancestors was serialized to disk.
  • serialize and deserialize the ancestor_ids using rails.
  • introduce a method to define these methods instead of including a module. This is the way that rails does most of its magic with attribute accessors.

@kbrock
Copy link
Collaborator Author

kbrock commented Jan 18, 2022

stalled: ancestry column becomes an array. so the concept of ancestor_ids and ancestry merge. Which I guess makes sense because the slash delimiter was just a database serialization artifact.

problem: lots of people expect the ancestry column to have a slash delimiter. So having it converted to an array will mess with them. Not sure how to be backwards compliant in this concept

# convert to database type
def serialize(value)
if value.kind_of?(Array)
value.map(&:to_s).join(@delimiter).presence
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure if we want an empty ancestry to have [] or nil

technically null means that the value is undetermined. But in our case, we want to say "none" which is "" and not nil.

@kbrock
Copy link
Collaborator Author

kbrock commented Jan 25, 2022

our main code base has move to using ancestor_ids in most places.
This helps us since having rails use a serializer or a a method don't matter as long as we are using int[] to access the columns.

I have had trouble bringing this up to date. Have tried a few times.

@kbrock
Copy link
Collaborator Author

kbrock commented Jan 27, 2023

surprisingly this has been difficult to fix the merge conflicts.

@kbrock
Copy link
Collaborator Author

kbrock commented Jul 13, 2023

this is imposible to rebase.
I have already pulled many of these concepts out of here.

Keeping around but will need to completely rewrite this when I get the chance

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