Skip to content
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

before_validation :update_permalink, on: :create is never triggered in an hyper model #218

Closed
lionelchauvin opened this issue Jul 19, 2019 · 6 comments
Labels
bug Something isn't working

Comments

@lionelchauvin
Copy link

lionelchauvin commented Jul 19, 2019

This is a common pattern to initialize some attributes before validation on creation. For instance acts_as_permalink does that: https://github.com/kmcphillips/acts_as_permalink/blob/55e824a5bf76f30ce8250dd9438bc46234e25b45/lib/concern.rb#L10

But in hyperstack, a new_record is saved without validation and validated later:

record.check_permission_with_acting_user(acting_user, op).save(validate: false) || true

The callback is never triggered because validation is applied to a record that is already created.

@catmando
Copy link
Contributor

catmando commented Aug 7, 2019

Interesting. Thanks for the report and investigation

@catmando
Copy link
Contributor

catmando commented Aug 7, 2019

The problem is that when saves are done from the client, hyperstack has to first "recreate" the context of all the records on the client. In some cases you can have validations that require some relationship exist, but these same relationships are already on the client. Hence we first save everything without validations, then roll back any things that are not true saves, then finally run the validations.

In looking into Rails validations, it appears we might be able to force on: :create validations to run, in which case the fix will be easier. Otherwise it's going to require some kind of monkey patching.

If you are interested in helping out, you could do a little sandbox experiment for us:

Try saving one of your records and then running valid?(on: :create) and see if it makes the callback. If so great. Then try and see if valid?(on: :save) works, then remove the validation and rerun the experiment.

In the meantime do you need a work around? I would suggest that you could use a ServerOp to do the save would be the best workaround.

@catmando catmando added this to To do in ALPHA to production via automation Aug 7, 2019
@catmando catmando added the bug Something isn't working label Aug 7, 2019
@lionelchauvin
Copy link
Author

The problem is that when saves are done from the client, hyperstack has to first "recreate" the context of all the records on the client. In some cases you can have validations that require some relationship exist, but these same relationships are already on the client. Hence we first save everything without validations, then roll back any things that are not true saves, then finally run the validations.

Client/server synchronization is an hard problem.
In any case, I don't think the client can be trusted, specially when unicity must be validated.

In looking into Rails validations, it appears we might be able to force on: :create validations to run, in which case the fix will be easier. Otherwise it's going to require some kind of monkey patching.

If you are interested in helping out, you could do a little sandbox experiment for us:

Try saving one of your records and then running valid?(on: :create) and see if it makes the callback. If so great. Then try and see if valid?(on: :save) works, then remove the validation and rerun the experiment.

I am sorry, I am currently on vacation. I will do that after the 19.

In the meantime do you need a work around? I would suggest that you could use a ServerOp to do the save would be the best workaround.

I prefer a durable solution. I think we should write some tests.

@catmando
Copy link
Contributor

catmando commented Aug 7, 2019

Client/server synchronization is an hard problem.
In any case, I don't think the client can be trusted, specially when unicity must be validated.

Sorry this was unclear. You are exactly right, all validations must be done on the server. So before doing any validations, we have to make sure that the current set of changes on the client are recreated on the server.

I am sorry, I am currently on vacation. I will do that after the 19.

No rush, it will be great to have help!

I think we should write some tests.

Really glad to hear that. With very few exceptions all changes to the code base come with tests, so creating the test specs is a good first step, or at least in parallel with seeking a solution!

@lionelchauvin
Copy link
Author

The problem is that when saves are done from the client, hyperstack has to first "recreate" the context of all the records on the client. In some cases you can have validations that require some relationship exist, but these same relationships are already on the client. Hence we first save everything without validations, then roll back any things that are not true saves, then finally run the validations.

In another project I designed a complex form system. I think I had a similar problem and I solved it this way:

  • load existing records and create new records for the others
  • assign attributes provided by the form
  • set all associations (some associations then contain new records)
  • travel through the dependency graph (nodes: records, edges: associations) in a specific order and save each record if it is changed.

Save a record without validation (like in hyperstack) should be done only when it is needed to break a dependency cycle. In most of the case it is not necessary.

Did you try something like that instead of save records twice ?

@catmando
Copy link
Contributor

If I understand you, this is close to what Hyperstack is doing, however to keep the programmer from doing extra work it has to do it in a general way without specific application knowledge. (I think you agree with that correct?)

As you point out the problem is cyclical dependencies requiring save's to be done first, and then validations.

I have no problem if you want to try to rewrite that core algorithm (it is in need of refactoring for sure) to use your approach.

HOWEVER...I would add this caution:

  1. currently we are saving everything then validating things that need it (two steps) The problem arises because this misses validations tagged for on: :create. I feel that given the problem is quite specific, the best fix might be to simply address that problem (i.e. force the on: :commit hooks to run)

  2. I believe the complexity of doing a pre-save tree traversal, will be equivelent to the current algorithm, and thus there will be no real savings in terms of run time or maintenance.

  3. If you did want to try this approach, I think the really best answer is to transfer all this logic down to the client (i.e. have the client compute the tree in such a way that when stuff gets to the server everything is done in the proper order without the server thinking about it. This would be best done as part of another issue (I think its already in the issue list) to clean up the client-server json structure to make it more transparent, easier to debug, etc.

Soooo... For now I would suggest getting the problem fixed by going after the on: :create simulation, and we can add another issue to do the bigger cleanup later.

lionelchauvin pushed a commit to lionelchauvin/hyperstack that referenced this issue Aug 20, 2019
…k, on: :create is never triggered in an hyper model
lionelchauvin pushed a commit to lionelchauvin/hyperstack that referenced this issue Aug 21, 2019
lionelchauvin pushed a commit to lionelchauvin/hyperstack that referenced this issue Aug 22, 2019
lionelchauvin pushed a commit to lionelchauvin/hyperstack that referenced this issue Aug 26, 2019
…: :create is never triggered in an hyper model
catmando added a commit that referenced this issue Aug 26, 2019
fix issue #218: before_validation :update_permalink, on: :create is n…
ALPHA to production automation moved this from To do to Done Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

No branches or pull requests

2 participants