-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
deprecate config repository use in web backend #19719
deprecate config repository use in web backend #19719
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I am just thinking that even if we are removing the repositiory, we will still do direct call to the DB in this class. It will just be hidden in the call to the handler.
/** | ||
* The web backend is an abstraction that allows the frontend to structure data in such a way that | ||
* it is easier for a react frontend to consume. It should NOT have direct access to the database. | ||
* It should operate exclusively by calling other endpoints that are exposed in the API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the last sentence is missleading because we are not calling other endpoints but other handler. Moving the calls we do to the repository to another handler will still add a direct call to the DB. If we want to call the other endpoint through this handler we should use the generated clients via open API (like io.airbyte.api.client.generated.HealthApi
). I don't think that calling the repository in this class will prevent us from having the server to be the gateway of all of our DB access since this handler is not use outside of the server module.
// todo (cgardens) - this handler should NOT have access to the db. only access via handler. | ||
private final ConfigRepository configRepository; | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue I saw when trying that is that IntelliJ don't give feedback about this deprecated (No strike through or red color). That's why I went with an abstract class and getter and setter since using this field will look like an error. That being say, the new naming is pretty explicit so I'm fine with both solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benmoriceau yeah. i noticed it too. good call. let's merge your PR since it gets the highlighting right. i'll merge mine after with the comment and the name change
done elsewhere. |
What
ConfigRepository
was ever added to the web backend.How