serenity-is / Serenity

Business Apps Made Simple with Asp.Net Core MVC / TypeScript
https://serenity.is
MIT License
2.57k stars 796 forks source link

FileUpload : timing of copying from temporary file to final file name is off? #6728

Closed LSNicholls closed 1 year ago

LSNicholls commented 1 year ago

Before submitting the bug report, please read and check the following items

What happened?

Steps to repro:

Do this in a new row/new form as part of an insert. I don't know for sure if the same thing happens on an update.

  1. Write an "after insert" trigger that takes the pathed file name from the row and populates it in a column in a different table.
  2. Upload a file
  3. Check the column in the other table
  4. Observed: the temporary file name and path are sent to the other table.

What did you expect to happen?

The "finalized" name and path of the file.

How to reproduce?

See "what happened" section.

What Serenity Nuget Versions are you seeing the problem on? (separated by comma)

6.5.1, MS SQL Server 2019

Relevant log output

No response

Serene template version

No response

Sergen version

No response

Code editor

No response

Operating System

No response

Node.js version

No response

TypeScript version

No response

Database type and version

No response

On which device do you see the problem?

No response

On which operating system do you see the problem?

No response

On which browsers do you see the problem?

No response

On what version of the browsers do you see the problem?

No response

Additional information

I fixed this by updating the values using AfterSave in the Save handler. But I was really surprised by this!

VictorTomaili commented 1 year ago

The FileUploadBehavior updates the name of the file in here https://github.com/serenity-is/Serenity/blob/master/src/Serenity.Net.Web/Upload/FileUploadBehavior.cs#L342

And the triggered here https://github.com/serenity-is/Serenity/blob/master/src/Serenity.Net.Services/RequestHandlers/Save/SaveRequestHandler.cs#L59

You should call your base function to trigger them, then you can access the latest file name in your save handler.

If you try to use this final name in the behavior, the call order will be effect the result. The problem is here, your behaviors should not be depended on other behavior data. There is no order functionality. Anyway, if it is a strict requirement, you can override the "GetBehaviors" in your SaveHandler and order them as required.

LSNicholls commented 1 year ago

OK thank you. But @VictorTomaili FWIW I wasn't "using the final name in the behavior". I was using it in a SQL trigger. I don't think this is the same thing. I understand that name change is being "triggered" in the AfterSave but was suggesting that maybe AfterSave was not the right place if a SQL trigger, which sits outside Serenity, was seeing the temporary name.

Also please note that my SQL trigger was after insert, not instead of insert.

There is no order functionality.

Yes, there is, in the sense that AfterSave is... after Save .

VictorTomaili commented 1 year ago

The SQL trigger is not expected in file update behavior. Serenity is not check for whether there is any trigger or not in SQL database. You have some business on your database side, and serenity is not responsible for that. You can move this business into your code, or you can check for the final state of the value with some extra business. Creating a new behavior for uploading files is also possible.

LSNicholls commented 1 year ago

@VictorTomaili of course Serenity isn't "responsible" for a trigger.

My point was exactly that a trigger is outside of Serenity, so I was surprised that it could see a temporary value.

If you are saying that all of this activity on the Serenity side up to and including the rename of the file and whatever else might occur during AfterSave, is part of a unit of work -- whether because the row is locked or a transaction is underway or some other definition -- from your point of view, and that nothing else from the outside should be touching the row until it is complete, then I understand your position.

If a transaction is underway at the time of AfterSave, it is really weird that the trigger sees the temporary value, in fact (remembering that it is an after, not instead of trigger.)

Are you absolutely sure that no other activity outside the Serenity process, beyond a trigger, couldn't accidentally read the temporary value from the database row, during a brief time period?

I want to understand how this works. If you are not absolutely sure, then I want to take care of it on my end, certainly not make Serenity "responsible" for it, but I also suggest that the docs reflect this behavior in some way.

Thank you in advance for understanding my position, and for a thoughtful response.