search

ferdium / ferdium-server

The server component that can either be self-hosted or hosted for general purpose - for use with the ferdium thick client.
MIT License
163 stars 41 forks source link

feat: Store iconUrl in workspace.data #102

Open orgrimarr opened 6 months ago

orgrimarr commented 6 months ago

Save iconUrl field within workspace.data

See https://github.com/ferdium/ferdium-app/pull/1240

SpecialAro commented 6 months ago

Just tested this and it seems not to be working. I'll investigate further

SpecialAro commented 6 months ago

Just changed the logic to add a column to the database. I think that way the code is way cleaner

We need to reproduce this change to the internal server as well

vraravam commented 6 months ago

Just changed the logic to add a column to the database. I think that way the code is way cleaner

agreed that it can be cleaner, but then as of this release, we will not be backwards-compatible with the other servers. Is that acceptable for us and the end users? Should we not ask for a poll in the Discord before we make such a breaking decision?

We need to reproduce this change to the internal server as well

yes, if we decide to go ahead with this, then the same change has to be done in the internal-server of the main repo AND we will need to do an in-tandem release of both the client and server at the same time. (Maybe we can release the server earlier - just got out of bed, and haven't thought that through)

SpecialAro commented 6 months ago

Just changed the logic to add a column to the database. I think that way the code is way cleaner

agreed that it can be cleaner, but then as of this release, we will not be backwards-compatible with the other servers. Is that acceptable for us and the end users? Should we not ask for a poll in the Discord before we make such a breaking decision?

I think we should, yes. Nevertheless, I feel that several things might be broken with Franz and Ferdi server (as we probably made some change with the Adonis migration that broke it). Otherwise, if you feel like we should store it inside the data object, I can find a way to make it work and also (hopefully) that doesn't break backward compatibility with Franz and Ferdi.

We need to reproduce this change to the internal server as well

yes, if we decide to go ahead with this, then the same change has to be done in the internal-server of the main repo AND we will need to do an in-tandem release of both the client and server at the same time. (Maybe we can release the server earlier - just got out of bed, and haven't thought that through)

I tried to reproduce the changes I made here in the internal server but I wasn't able to get the migrations to work... If we move with the approach of adding a new column would you be able to help me on that?

If we store the iconUrl in the data column then disregard what I said previously... I'm ok with this decision as well 😁 we just need to refactor the Ferdium-app PR as well to account for this change.

orgrimarr commented 6 months ago

Thought I don't have a clear overview of ferdium architecture but, Is this really a breaking changes to add a new column ?

If I understood correctly, the contract between ferdium client and backends are the API. So we can imagine storing the data differently on the backends.

Looking at the current API implementation state. New fields are ignored. So I should not break others backends.

So, if there is a breaking, it's at the API level, not the database schema level.

vraravam commented 6 months ago

If I understood correctly, the contract between ferdium client and backends are the API. So we can imagine storing the data differently on the backends.

Technically, you are 100% correct. Extra fields/columns will be ignored.

Consider this scenario

  1. an existing user on Franz server exports and switches to the Ferdium server - all with the Ferdium client. Then the new column is (as expected null). They then find the Ferdium server unusable for their needs (there could be many reasons - 1 of them for eg that Franz server has some team management capabilities which we do not support in either the client or the server). In the meantime, if they have set an icon url, and export, this field comes into the exported data. Now, if they import back into Franz, they will lose such functionality. imo, this is "breaking" in nature. I am not opposed to this per se - just that this needs to be clearly called out as part of the release notes and FAQ, etc so that the user is not blind-sided
  2. Consider the reverse of the above. Ferdium server, icon url is set, the user then decides to move to one of the older servers, exported data contains the icon, importing into the old server will "lose" such data. Eventual re-importing will still mean the iconUrl data is lost - unless they make the decision to reimport from the ferdium-export rather than re-exporting from the old server. The trade-off will be if they have made any changes to any storable info in that interim, they either lose that data or the iconUrl data - their choice.