Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
v1 Scheduled Jobs / Third Party Sync Migration #171
base: master
Are you sure you want to change the base?
v1 Scheduled Jobs / Third Party Sync Migration #171
Changes from 31 commits
0d314b6
7aaed92
9edb9fe
fd73ff5
3fb087b
2b17972
d1cba21
3e49061
6a6d61c
2c2d80c
6bbe94d
ae0fff4
8dea68d
427b4f0
14ba12b
4aeaccf
85f141d
30dac28
9a42a6b
4ce23ff
3efecaf
246597e
348a15d
bcaef3f
f44b540
40ddd6f
7f41168
94208dd
fff8a9f
fc8cdaf
682c2bc
60277d6
09135bc
d97ceb3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Out of curiosity - is the use of @transient here to do with some desired serialization behaviour?
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.
Yes that's exactly what its for in this instance, the transient here indicates to the JPA that the field should not be persisted to the database and when the row is pulled from the db, the deserializeProperties method in the entity is called as it has the PostLoad annotation. The properties string from the db is deserialized into this transient field. The setter of this transient field converts itself back into a JSON string which is stored in the propertiesString field that is persisted to the db.
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 don't see this column definition used in the repo before. Is it necessary?
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.
Also is the nullable false necessary since you default the field value to false?
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.
Ideally, we would use the "BIT" column type here - as that is the closest semantical equivalent for storing a bool in MySQL and is the smallest amount of space used.
https://dev.mysql.com/doc/refman/8.0/en/bit-type.html
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.
For the actual entity class names - we don't tend to suffix them with the word "Entity", e.g.:
public class Locale extends BaseEntity
(vs.LocaleEntity
)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 ended up using Entity for the suffix here as I have another class labelled ScheduledJobType in another package that I created (You will see it further on in the PR). Would the best approach be to rename that ScheduledJobType to something else so that the entity version can use it ? I'm thinking of calling the current one : ScheduledJobTypeEnum as the enum currently uses that name.
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 id here should probably be of type
long
instead ofstring
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 id here is a UUID. However, I have changed the data type here to be a UUID so that Spring will respond with a 400 Bad Request if it isn't a valid UUID passed through.
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.
You get the scheduler 3 distinct times in this method, would it not be possible to store it in a variable and just reuse the result? Or does the scheduler vary a lot and you need to wait to the last possible minute to get it each time?
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.
Nit: toggle is a strange term for this. I get that you mean it disable and enable a job, but this name feels confusing
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.
Honestly I have a preference for separate endpoints for enable and disable because this current endpoint expects the called to know what the previous state was. This could make debugging very difficult.
Example: I called toggled on a job, but I used the wrong job's id. Now the output could either be that it is toggle or not toggled depending on its prior state, which we don't know since this was a mistake. It adds a layer of "what was it before? because that dictates what it is now". Where as the if the endpoint is
/enable
you know if the status code is 200, then it is enabled now. No confusion, no mental gymnastics requiredThere 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.
Sounds good, I'll change it to two separate endpoints!