GenieFramework / SearchLight.jl

ORM layer for Genie.jl, the highly productive Julia web framework
https://genieframework.com
MIT License
139 stars 16 forks source link

First commit and adding Postgresql-Test #28

Closed FrankUrbach closed 2 years ago

FrankUrbach commented 3 years ago

Hallo Adrian

Your PostgreSQL-SearchLight-Adapter deserves a little bit of tests. For this I have prepared the file test_postgreSQL.jl. In this file I have done some teardown-statements for more independence of the test cases. Feel free to use this file for your work. Because of differences between MySQL and Postgresql it can be necessary to build some api in SearchLight for the uniform treatment of different kinds of databases. If this is, what you have in mind, please let me know. On the journey through my application I will add some features in the api for different databases.

Best, Frank

essenciary commented 3 years ago

@FrankUrbach Thanks for these, this is great! I'm thinking maybe they'd make more sense in the SearchLightPostgresql adapter package? This way each adapter will contain its own tests, instead of including everything in core SearchLight (which can have some generic tests of its own).

FrankUrbach commented 3 years ago

Hallo Adrian That make sense. I would prepare this if I come back from my vacation. What do you think about the integration of the tests for MySQL in the SearchlightMySQL-Adapter? In my opinion the definition of a standard testset which all adapter should fulfill is very useful. Also the cleaning of items done in the database during the tests should be done. First teardowns I’ve made in the tests for the SearchlightPostgres-tests. Best Frank

essenciary commented 3 years ago

@FrankUrbach that would be fantastic! I also believe that would be the best approach (tests with the adapter). SearchLight itself can include some generic unit tests which are not backend related.

FrankUrbach commented 3 years ago

Hi Adrian I've added some functionality for getting a table_create_migration out of an DataType. In some circumstances it will be a little bit tedious to write all of them by hand. This could be error prone and not that handy as it could be. I'm very interested in your opinion. Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian I've added some more things for getting things in the right direction. Hope you will find the time to read the changes and can commit it to the official branch.

Keep healthy.

Best Frank

essenciary commented 3 years ago

Thanks @FrankUrbach that's pretty cool stuff! I hope I understood well your intentions re the code and I've provided some feedback. 😊

FrankUrbach commented 3 years ago

If you use my whole chain from SearchLight and SearchLightPostgreSQL you will find in the tests exactly this. I don't think we should use an extra struct. This would bring more complexity to the whole thing. Instead of this read more about my thoughts below. But for now it is only possible to use variables which should be stored in the database. Now I think about a system where it is possible to set a conversation between stored columns in a database and the fields in a struct. I branched my own repo to try this and show what I mean. For now I think about a simple array of tuples in which one side shows the fields of the struct which should be stored and the other side the column_name in which the data should be stored in the database. So it is possible to decouple the model-side from the database-side which is very useful for situations where we have not the chance to live in an environment where we haven't the freedom to determine what is going on in the tables. In order not to make it too difficult for a beginner at this point, the function should return the fields of the struct as default. The rest is straight forward to implement this feature in read, write and update the database. As I said: More soon in code. I will inform you, if I have something ready for review.

essenciary commented 3 years ago

The idea is not to use an extra struct - we use the model itself, which we define in the application (we can define the model first, then run the migration based on it). We do have mappings already, for each backend, eg: https://github.com/GenieFramework/SearchLightPostgreSQL.jl/blob/cbbd7b5626965d29653edd6987afde9d4f62699e/src/SearchLightPostgreSQL.jl#L23

FrankUrbach commented 3 years ago

That's not the the point I meant. The point here is e.g. you have a struct with a field "sizeA" and want store this in the column "size_a" and you don't want this in your model. So you have to tell the model somehow how to store this. This lives beside the type conversation we have for each adapter. As an example:

fields: a , b, c , d, e. where a, c, e are intern fields and b and e should store in the database. columns(Database): r, z the mapping for the storage is: b =>r e=>z. A type conversation is not planned because this would lead to not debugable code.

essenciary commented 3 years ago

SearchLight already ignores fields that don't have a corresponding column by applying a default mapping convention (from field name to column name). Indeed, explicit mapping would be very useful in order to support column names that don't follow SearchLight's conventions (especially legacy code).

FrankUrbach commented 3 years ago

"SearchLight already ignores fields that don't have a corresponding column by applying a default mapping convention (from field name to column name)." I thought, this is the the case but if I'm using a struct with fields not included in the set of database columns saving the object go's wrong. Perhaps I'm wrong but in my tests the behavior you described is not there for an insert aka save.

FrankUrbach commented 3 years ago

Hallo Adrian I've added a branch to my fork of SearchLight and SearchLightPostgreSQL called "saveWithoutInterns". For this I added a function storeableFields to SearchLight and imported this in the PostgreSQL-adapter. With this technic it is possible to use it over the import in a module to use it as an interface. If the function is not implemented in a model there is a fallback function in the adapter.It would be very useful If you have the time to review the code bevor I merge it with my master. Any hints and comments are very appreciated.

Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian Now it's accomplished. In the branch mentioned above the possibility of insert, update and a simple find is implemented. Works like a charm. The patch for the issue #31 is also there. If you find some time, you can review the code. For now it is only possible to see this in conjunction with the SearchLightpostgreSQL-Adapter with the same branch name. If we have discussed the solution I will transfer the required parts to the SearchLightMySQL-adapter.

Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian As you mentioned I have changed the tests in a way that they run form the runtests.jl within in the tests-folder. It took me some time to figure out one error I introduced by myself. But now all tests are green in the "saveWithoutInterns"-branch. Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian Now it is possible to store, read and update models where the intern fields not the same as in the database. For now more difficult queries from the database are not included. Any more complex models were appreciated. The next step on my journey is to obtain the possibility to store and read Models with fields containing AbstractModels or arrays of them. Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian Next step accomplished. In the "saveWithoutInterns"-branch now it is possible to store fields with abstract models or arrays of abstract models. The next step is to read these items. But for the moment it is only possible to insert these items. For the update branch of the save method more work is needed. This should happen in the next days. Best Frank

essenciary commented 3 years ago

Hi @FrankUrbach - thanks so much! Sorry, I have been super busy and haven't had the chance to work on the project lately. I'll take a look ASAP.

FrankUrbach commented 3 years ago

Hi Adrian I think, now it is done. In the branch described above you can see a full working spec for saving and finding AbstractModels with fields containing abstract models and with the ability to save the fields with different names in the database. I haven't figured the ability to store in a different table than the original. I theory it should be possible. This all work for now only with the PostgreSQL adapter. The next step it is to port this to the MySQL adapter. Not the nicest job, but it should be done. Best Frank

FrankUrbach commented 3 years ago

Hallo Adrian It went faster than I thought. If you go to the branch saveWithoutInterns in my fork of SearchLightMySQL.jl you will find the same tests as in the PostgreSQL-adapter. It brings the same functionality and all tests are green. I think, job done :-)). For now I will go to work on my application using Genie, Stipple and StippleUI until you have the time to review the code. No pressure. I hope my code will be appreciated by you. But have in mind: I'm a Julia beginner. Not all design decisions will be in the "Julian" way and sometimes maybe not what you imagine for some points. Sometimes we could use the broadcast technic more. But in my humble opinion the code is sometimes better readable without this. In a nutshell what now is possible:

Because of the using recursive algorithms saving and reading model structures with a random depth should be possible. There are some rough edges in regards of the conventions for submodels id but this should be possible to figure out in the near future.

Best Frank

FrankUrbach commented 2 years ago

Hi Adrian

This is my last attempt to ask you if you want incorporate the ideas underlying in this PR. You moved forward to version 2.0 without we discussing anything here. I made more then once a invitation to discuss some ideas which came in my mind during the work on the Postgres adapter. I didn't want sound rood but I think to keep open this PR isn't longer useful. I notice with that you didn't want help in this project so I make my conclusions for myself. Sad but it is how it is.

Best Frank

FrankUrbach commented 2 years ago

The pull request is obsolet because of other development directions during the time it was open.