Location#merge fix

[JoeCohen]

Prevents certain broken references, i.e. references to Locations that are deleted via a merge:

• Updates herbarium.location if the location has been merged.
• Ditto for project.location

Fixes the bug described in #2084

[coveralls]

coverage: 94.402% (+0.001%) from 94.401% when pulling 71576ba9f884de92cdb4a3b4e9388d32a4bdaba7 on jdc-2084-locationmerge-incomplete into dfa9ac66dac2e48134d911e0b50aae281c73cdb0 on main.

[JoeCohen]

@mo-nathan, @nimmolo, @pellaea : NOTE: Location#merge is a hidden trap for the unwary when adding a location to an existing model or when adding a new model that has a location.

Question

Is there a way to either

• Reorganize the code to avoid this trap?
• Detect if an existing or new model has fallen into the trap and warn the coder?

[mo-nathan]

By "trap" I think you mean that it's easy to not realize that Location#merge needs to be aware of any relationship to a Location object in case that object is deleted/merged, right? If that's what you mean, you could at least look at Location.reflection. However, in this case it would still have missed Project since Project has a belongs_to relationship with Locations, but this relationship is not expressed in location.rb with a corresponding has_many. If you wanted to be through along the lines of check_for_broken_references, you could go through all the reflections of all the models. Getting all the models is a bit tricky due to Rails lazy loading. Googling around a bit, it seems like you might have to do some eager loading or file searching in the models directory. If you do want to go through all the models, you could also (at least in theory) determine if for every belongs_to there is corresponding has_many or other relationship.

[pellaea]

My two cents: check_for_broken_references kinda accomplishes the "detect if something has fallen into the trap" thing. Say you add another reference to location and forget to deal with it in Location.merge. Then when such a referenced location is merged, check_for_broken_references will let us know. Somewhat cryptically, I admit, but it will point us in the right direction, in any case.

On Tue, Apr 2, 2024 at 9:55 PM Nathan Wilson @.***> wrote:

By "trap" I think you mean that it's easy to not realize that Location#merge needs to be aware of any relationship to a Location object in case that object is deleted/merged, right? If that's what you mean, you could at least look at Location.reflection. However, in this case it would still have missed Project since Project has a belongs_to relationship with Locations, but this relationship is not expressed in location.rb with a corresponding has_many. If you wanted to be through along the lines of check_for_broken_references, you could go through all the reflections of all the models. Getting all the models is a bit tricky due to Rails lazy loading. Googling around a bit, it seems like you might have to do some eager loading or file searching in the models directory. If you do want to go through all the models, you could also (at least in theory) determine if for every belongs_to there is corresponding has_many or other relationship.

— Reply to this email directly, view it on GitHub https://github.com/MushroomObserver/mushroom-observer/pull/2085#issuecomment-2033397303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYTNNNAN24P4XDWATMFR73Y3NOSBAVCNFSM6AAAAABFUHEOF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGM4TOMZQGM . You are receiving this because you were mentioned.Message ID: @.***>

[JoeCohen]

Thanks Nathan!.

1. That's exactly what I meant by ""trap for the unwary". E.g., We've neglected Herbaria locations for years. And the Project location_id column was added, there weren't corresponding changes to Location merge.
2. I just pushed a test that finds the models via file searching. But my test isn't clever about reflection or other relations. At the moment it seems like too much trouble for now. Probably -- as @pellaea observes -- script/check_for_broken_references is enough to do what we need.

On Tue, Apr 2, 2024 at 6:55 PM Nathan Wilson @.***> wrote:

By "trap" I think you mean that it's easy to not realize that Location#merge needs to be aware of any relationship to a Location object in case that object is deleted/merged, right? If that's what you mean, you could at least look at Location.reflection. However, in this case it would still have missed Project since Project has a belongs_to relationship with Locations, but this relationship is not expressed in location.rb with a corresponding has_many. If you wanted to be through along the lines of check_for_broken_references, you could go through all the reflections of all the models. Getting all the models is a bit tricky due to Rails lazy loading. Googling around a bit, it seems like you might have to do some eager loading or file searching in the models directory. If you do want to go through all the models, you could also (at least in theory) determine if for every belongs_to there is corresponding has_many or other relationship.

— Reply to this email directly, view it on GitHub https://github.com/MushroomObserver/mushroom-observer/pull/2085#issuecomment-2033397303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALDFBGV53SCSUHTMV5H63Y3NOSBAVCNFSM6AAAAABFUHEOF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGM4TOMZQGM . You are receiving this because you authored the thread.Message ID: @.***>

[pellaea]

I should back-pedal a bit. Even my script relies on a belongs_to relationship being declared somewhere, which doesn't necessarily have to happen.

(Incidentally, my script is slightly better than grepping for "belongs_to", but it still does rely on directory listing of app/models to find the full list of model classes! Once it has a list of model classes, it uses a documented ActiveRecord trick for enumerating the reflections, so that's safe and stable.)

On Tue, Apr 2, 2024 at 10:07 PM Joseph D. Cohen @.***> wrote:

Thanks Nathan!.

1. That's exactly what I meant by ""trap for the unwary". E.g., We've neglected Herbaria locations for years. And the Project location_id column was added, there weren't corresponding changes to Location merge.
2. I just pushed a test that finds the models via file searching. But my test isn't clever about reflection or other relations. At the moment it seems like too much trouble for now. Probably -- as @pellaea observes -- script/check_for_broken_references is enough to do what we need.

On Tue, Apr 2, 2024 at 6:55 PM Nathan Wilson @.***> wrote:

By "trap" I think you mean that it's easy to not realize that Location#merge needs to be aware of any relationship to a Location object in case that object is deleted/merged, right? If that's what you mean, you could at least look at Location.reflection. However, in this case it would still have missed Project since Project has a belongs_to relationship with Locations, but this relationship is not expressed in location.rb with a corresponding has_many. If you wanted to be through along the lines of check_for_broken_references, you could go through all the reflections of all the models. Getting all the models is a bit tricky due to Rails lazy loading. Googling around a bit, it seems like you might have to do some eager loading or file searching in the models directory. If you do want to go through all the models, you could also (at least in theory) determine if for every belongs_to there is corresponding has_many or other relationship.

— Reply to this email directly, view it on GitHub < https://github.com/MushroomObserver/mushroom-observer/pull/2085#issuecomment-2033397303>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAALDFBGV53SCSUHTMV5H63Y3NOSBAVCNFSM6AAAAABFUHEOF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGM4TOMZQGM>

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/MushroomObserver/mushroom-observer/pull/2085#issuecomment-2033405572, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYTNNISUFMZDKM4VQY6X7DY3NP6PAVCNFSM6AAAAABFUHEOF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGQYDKNJXGI . You are receiving this because you were mentioned.Message ID: @.***>

[nimmolo]

So then...how about just adding at least this to Location.rb — the one that we know about?

has_many :projects

No sense putting it off til we have the exhaustive list of missing reflections.