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

Columns createdBy, createdDate, lastModifiedBy and lastModifiedDate should be filled automatically #236

Closed
glutengo opened this issue Jun 7, 2021 · 8 comments · Fixed by glutengo/generator-jhipster-nodejs#2 or #237
Assignees
Labels
$$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ enhancement New feature or request Feature New feature v2.0.0 Stable version with jhipster 7.0.0 $200 https://www.jhipster.tech/bug-bounties/
Milestone

Comments

@glutengo
Copy link
Contributor

glutengo commented Jun 7, 2021

Is your feature request related to a problem? Please describe.
The base entity has the mentioned columns. However, these columns are never used when a new entity is written to the database, so they always have null as value.

Describe the solution you'd like
These columns should be filled automatically when an entity is saved. For the date columns, this could be achived by replacing the Column() decorator with CreateDateColumn() and UpdateDateColumn()(as describe here). For the user columns this could be handled somewhere in the entity service.

glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
@ghost ghost assigned glutengo Jun 7, 2021
@ghost ghost added enhancement New feature or request Feature New feature labels Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue Jun 7, 2021
@glutengo
Copy link
Contributor Author

glutengo commented Jun 8, 2021

Quick status update on this:

  • Storing the created / last modified date was pretty simple and worked as expected.
  • For storing the user, my initial approach was to inject the current request into the relevant service (namely UserService and EntityService), read the current user from the request (this has been set previously by the Passport JwtStrategy I think). But there are some issues:
    • If I do that, the affected services are re-generated for every request. This could have some impacts on performance (see here)
    • In addition to that, this breaks the passport authentication strategy, as documented here. I was able to work around this by using request-scoped authentication strategies. I did make it past the e2e server tests and it worked for a locally generated application on my machine, but the e2e client tests with production builds are still broken with the error message from the mentioned issue.

I see three options to continue now:

  • Further investigate why the client e2e tests are failing and find a solution. However the used setup might have performance impacts, so I am not sure if this is the right thing to do
  • Do no try to inject the request in the services but pass the request (or the user) as a method parameter to the required methods instead. This is probably the safest way to go, but it will bloat the method signatures of many methods of the mentioned services
  • Try to use a different mechanism like zone.js, cls_hooked or async_hooks as described here. This seems quite interesting but would mean that a new library is added (in case of zone.js) or we use an experimental API feature which may be subject to change in the future.

@amanganiello90 what do you think? Any suggestions?

@ghost
Copy link

ghost commented Jun 8, 2021

HI @glutengo you are rock!! Great work!!

However I don't understand your goal. Have you implemented the createdBy, lastModifyed and so on filled automatically? That is the issue.

Now why you want to manipulate user and store it in the request?

@glutengo
Copy link
Contributor Author

glutengo commented Jun 8, 2021

Hi @amanganiello90, thanks for getting back!

I have been able to implement createdDate and lastModifiedDate. I have not managed to implement createdBy and lastModifiedBy yet because of the problems I described above.

@ghost
Copy link

ghost commented Jun 8, 2021

Ok, So I consider to use zone.js , because it is a thread-safe local storage. In this way you can store user and fill the createBy and lastModifiedBy fields.

@ghost
Copy link

ghost commented Jun 8, 2021

I really appreciate your dedication to work in these days. So @mraible could we add a bug bounty on this issue (or on #214) in order to give a price for @glutengo collaboration? Thanks a lot

@glutengo
Copy link
Contributor Author

glutengo commented Jun 8, 2021

@amanganiello90 thanks for your appreciation! I have found a solution now which works without request-scoped providers and without zone.js. I have adjusted the method signatures for the save and update methods of the services so that a creator / updater can be passed. I will open a PR for that now.

The NestJS lead developer has prepared a PR for integrating the async_hook API a while ago but refuses to merge it due to the experimental state of the API. I suggest to watch this and make use of it once it has been merged: nestjs/nest#1407

@ghost ghost linked a pull request Jun 8, 2021 that will close this issue
@mraible mraible added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $200 https://www.jhipster.tech/bug-bounties/ labels Jun 8, 2021
@ghost ghost closed this as completed in #237 Jun 9, 2021
@ghost
Copy link

ghost commented Jun 9, 2021

@glutengo great job! Now you can receive your bug bounty according the https://www.jhipster.tech/bug-bounties/ .
Thanks, you rock !!

@ghost ghost added this to the 2.0.0 milestone Jun 10, 2021
@ghost ghost added the v2.0.0 Stable version with jhipster 7.0.0 label Jun 10, 2021
@glutengo
Copy link
Contributor Author

$200 bug bounty claim: https://opencollective.com/generator-jhipster/expenses/43423

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
$$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ enhancement New feature or request Feature New feature v2.0.0 Stable version with jhipster 7.0.0 $200 https://www.jhipster.tech/bug-bounties/
Projects
None yet
2 participants