skiedude / evestructures

An ESI/SSO backed website enabling Eve Online Corporations to view and manage all their owned Structures in New Eden
https://structures.eveskillboard.com
MIT License
22 stars 12 forks source link

[discussion] Database table design issues #4

Closed IvandaNothabeer closed 6 years ago

IvandaNothabeer commented 6 years ago

Note: These are issues that only become really apparent when adding an Admin back end and trying to manage structures for multiple users.

So there are some problems that appear to make the database a bit bigger than it needs to be, specifically with the "structures" table....

In the Blade view, the view is reading every structure and then displaying only the structures owned by a user. As the number of structures increases in the database, eventually the view is likely to run out of memory trying to pull every structure.

When trying to admin all the Structures in the database, it's required to group the Structure records by structure id to eliminate the duplicated structures. Which is extra unnecessary work when the database should be doing this.

Suggestion: Link the Users table to the structures table with a HAS_MANY relationship and search for users->structures in the home view

The same issue exists with the structure_services table. Because there are duplicated structures, there are also duplicated structure_services. eliminating the duplicate structures will eliminated the duplicate structure_services. Ditto for structure_states and structure_vuls

skiedude commented 6 years ago

So I did that on purpose. I wanted to make it so 2+ characters from the same corporation could have a separate account, and not necessarily share the data. It also prevented one user from delete their account/character and deleting the shared structures between their 2 accounts.

The blade should only be passed the structures attached to that user https://github.com/skiedude/evestructures/blob/a78134eb9701b1e6e9f5ed0f1a1cb9685d5ee6de/app/Http/Controllers/HomeController.php#L39 Maybe I misunderstood your comment about All structures are being passed to the blade.

IvandaNothabeer commented 6 years ago

Sorry - I missed the call to auth in HomeController. You are indeed correct - That works just fine 👍

The table structure I think should be ... User HAS_MANY Characters Characters HAS_MANY Structures Structures HAS_MANY Characters

Characters and structures are linked through a pivot table: characters_structures. There's a good write up of pivot tables here ... http://laraveldaily.com/pivot-tables-and-many-to-many-relationships/ including how to keep them properly synched when you add and remove things.

Users -> Characters <-> character_structures <-> Structures -> structure_services etc

When you retrieve the User, you get their characters and structures as related records. So the HomeController just needs to find the User.

Yes, deleting is an issue. However, I think you can use the "updated_at" to remove any structures more than say 14 days old in the database. This will catch any orphaned structures that didn't already get deleted when the final user who had a relationship to the structure is deleted.

skiedude commented 6 years ago

Alright, I'll hopefully get time to read that over the weekend and see if I can get it working on my dev site. If you already have this figured out, feel free to just make a PR I can test out

IvandaNothabeer commented 6 years ago

I've already forked and made a start ....

  1. The MySQL tables characters and structures must be Innoddb. Change database engine if required

  2. Add the generators to artisan

    composer require laracasts/generators --dev add the service provider definition to AppServiceProvider.php

  3. Create the pivot table ...

php artisan make:migration:pivot characters structures fix the Migration and correct the model name in App\Database\Migrations then run the migration php artisan migrate

  1. Add the relation code to App\Character...

    /**
    * The structures that belong to the character.
    */
    public function structures()
    {
        return $this->belongsToMany('App\Structure');
    }
  2. The pivot table is empty. Use phpmyadmin to create new character_structure records linking character_id to structure_id or write code to sync all existing characters and structures

You can now fetch characters with their related structures ... Character::with('structures')->get();

IvandaNothabeer commented 6 years ago

Heh, its turning into a bit of a major job. :) Gimee a few more hours

skiedude commented 6 years ago

I was thinking about this today and I wonder if it would be far easier to just alter the characters table to hold the corp id of the character and then just key off the corp ID in the structures table. Then just join all the structure* tables off the structure id. Then you don’t need a pivot table and the deletion check that you do is if any characters exist with the structures corp id.

That seems like a lot smaller of a change then what you're probably going through @IvandaNothabeer

skiedude commented 6 years ago

With my proposed change it should just require some simple database migrations, updating the models, and making a new job that runs once a week looking for structures with no characters with matching corp ids.

I should have time tomorrow afternoon/night to try it out

IvandaNothabeer commented 6 years ago

Its done. You can pull from the IvandaNothabeer / EveStructures fork

Most of the changes are in the ESI interface to get the updates running properly. (Which is why it turned into a bigger job than expected :) - overall it wasnt that hard ) I've also removed un-needed columns from the structures tables. The Database Migration doesn't have a "back" because there's no way to restore deleted columns. So you may want to backup your database before you try this.

There's still some final tidying up to be done - mainly the cleanup of unused structures. However, Updates and Fuel Notifications should all be working.

The ESI update now "touches" every record - so the "updated_at" will be correct even if the record has not changed. if an updated_at is a few days old, then there is no character left owning the structure.

E: the advantage of using a pivot table is you can use the Attach, Detach and Synch methods to manage the MANY-MANY relationship.

skiedude commented 6 years ago

Anyway to only get the changes related to this discussion? You have a lot more customization in your fork then what I'd like to pull in (readme, roles, etc)

IvandaNothabeer commented 6 years ago

ignore /route/api.php unless you want to implement a REST API - For a public site you would need to add an individual Bearer Token to the users profile and verify that.

That should be it. The main thing is removing voyager and not running the migrate until that's done. Without Voyager, there's no back end.

skiedude commented 6 years ago

I've been reading over your changes and as I saw what you were doing, I thought I could achieve the same without a pivot table and reducing the amount of rewrite (thanks for the inspiration!).

Check out https://github.com/skiedude/evestructures/commit/a65b98467cdbf2d2bf1960746271e5f6ee87e20d (branch database_update)

Now we key off the characters.corporation_id and the structures.corporation_id, all the structure* just join off the structure_id. Anytime we are going to be displaying info, we still do a check for is_manager = TRUE. This prevents someone from just adding a character in the same corporation on a separate account and seeing all the structures without the proper roles.

When a delete happens we only delete the character entry, I still need to write a job to clean up stale structures that don't have a character with a matching corp_id and is_manager = TRUE.

And let me know what you think.

IvandaNothabeer commented 6 years ago

You need a pivot table. The pivot table implements and stores many_to_many connections in BOTH directions. A structure has many characters AND a character has many structures.

This prevents someone from just adding a character in the same corporation on a separate account and seeing all the structures without the proper roles.

You don't need a roles check. The pivot table does this. In order to create the connection between chars and structures you must Pull via the ESI and populate the pivot table. If you don't have the corp roles, you can't create a link in the pivot table and you cant see the structures.

I do agree that the edge case where the character loses roles needs to be addressed. If the Char does not have the station manager role, then any existing Structures need to be detached by removing the pivot table entries for that char.

( e: Btw, Fuel Notifications is a good example of where you search all the structures looking for their related characters.)

skiedude commented 6 years ago

I get where you're coming from, but I'm not sold on the pivot table. I'd rather just have a hasMany on each the structure and the character model.

If you look at https://github.com/skiedude/evestructures/commit/88a44e670e4e64bd75cdeff70313bc0fb62fa05e

I've update the models to have a different primary key so that the hasMany can join off the structure_id for structures and the corporation_id for characters. Now I can get all the characters that connect to the structure.

` public function characters() { return $this->hasMany('App\Character', 'corporation_id', 'corporation_id'); }

foreach ($structures as $structure) { $characters = Structure::find($structure->structure_id)->characters; foreach ($characters as $character) { `

skiedude commented 6 years ago

I added the new orphan structure job as well. At this point its good to go.

IvandaNothabeer commented 6 years ago

You NEED a pivot table. Without one you will require either duplicate structures or duplicate characters.

Explanation ... Without a pivot table:

Character_1 owns structures_1 , _2 & _3 ..Character_1 is related to the structure via the character_id field of the structures, so character_id=1

Character 2 is in the same corp, so it also owns structures_1 , _2 & _3 ..Character 2 links to the structures how ? - The structure already has the character_id = 1

That's why you need a pivot table.

skiedude commented 6 years ago

Well I'm pretty sure I have achieved the same functionality without a pivot table. I dropped the user_id,character_id from the structures table and now I associate a structure with a character by the characters.corporation_id = structures.corporation_id.

I was already doing a roles check as a precautionary measure to not generate more errors hitting endpoints so I simply added 2 lines of code to that block to set the is_manager column to TRUE/FALSE depending on case.

I then added a simple && char->is_manager == TRUE in the blade to enforce only showing structures for characters that have that.

I have a hasMany relationship boths ways between characters and structures using the corporation_id. This is probably wrong by industry standards, but I prefer it over a pivot table.

I also updated the models, and have a hasMany relationship between the structure and all the services/states/vuls (I also removed the character_id column from all of those).

As it stands with my testing I have multiple characters from the same corporation on the same account, different accounts and no duplicate structures are ever created. Deleting a character or account doesn't delete structures. I wrote a seperate job that runs once a week that checks for structures without any characters of the same corporation_id

IvandaNothabeer commented 6 years ago

edited:

So you linked by corporation....

Corporation HAS_MANY Structures Structure HAS_ONE Corporation

skiedude commented 6 years ago

I did test that. The way I'm making the structure entry is using an updateOrCreate and I key off the structure_id, corporation_id. So even if its separate accounts, it won't matter since its only keying off the structure_id and corporation_id. It has no knowledge or does not care about which account its on.

IvandaNothabeer commented 6 years ago

Sorry, you beat me to my edit :)

The Characters table is acting as a pivot table and using Joins to get structure.characters. And in the process using a bunch of extra code to do it.

skiedude commented 6 years ago

Yeah thats probably right, I'm not using an official pivot table, just created my own frankenstein. Which I'm happy with. Unless I find a bug or breaking change with my code when I test it some more tonight I think I will be heading the main repo using this variation of everything we have discussed.

Since you're on your fork you can happily still use your variation.

One thing I thought of is I need to not update a structure multiple times if a different character already did, should just be able to check the updated_at.

I thank you for bringing this to my attention and helping me work through this mentally, even though we branched in different directions.

skiedude commented 6 years ago

Also I have a discord in the forums post for this tool, feel free to hop on there as well to chat without having to wait for these issues

skiedude commented 6 years ago

I'll close this discussion for now. I think we made good progress and we've finalized our own solutions. Cheers