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

Add InteractionScope attribute. #10

Open
kegor opened this issue Mar 16, 2019 · 6 comments
Open

Add InteractionScope attribute. #10

kegor opened this issue Mar 16, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request important

Comments

@kegor
Copy link
Contributor

kegor commented Mar 16, 2019

Was initially reported by @mtornwall:

Startup has attribute [UseDbScope] which is used to say whether view model should be wrapped in Db.Scope or not. So [UseDbScope(false)] => not wrapped in scope and vice versa.

But the behavior of Db.Scope() is such that if there already is a scope allocated for an outer view model (or blended view model, IIRC), no new scope will be created for the nested view model. Instead it will be folded into the outer scope. However, sometimes you want to explicitly request that a new Db.Scope() be created. This could be done with new Transaction(....).Scope(() => ...)

I would like a new attribute, with options such as:

[InteractionScope(InteractionScopeMode.None)]

...

enum InteractionScopeMode {
      None = 0,
      Create = 0x1,
      Attach = 0x2 
}

Where their meaning would be:
None => view model should not be attached to any scope
Create only => view model should always get a new scope as above
Attach only => view model should attach to existing scope, if any, but not create one
Create | Attach => view model should attach to existing scope if any, otherwise create a new one

@kegor kegor added the enhancement New feature or request label Mar 16, 2019
@kegor
Copy link
Contributor Author

kegor commented Mar 17, 2019

@mtornwall what i found here:

  1. DbScopeMiddleware seems obsolete and MasterPageMiddleware is now responsible for creating a scope: https://github.com/Starcounter/Starcounter.Startup/blob/master/Starcounter.Startup/Routing/RouterServiceCollectionExtensions.cs#L29
    As i can see the app that now use [UseDbScope(false)] don't use AddDbScopeMiddleware (the same situation i saw during debug of the UserAdmin-app, during calling the pages with or without [UseDbScope(false)] only MasterPageMiddleware was called).
  2. As was mentioned above now MasterPageMiddleware uses Db.Scope() for wrapping non-partial requests (and it is called from the AddRouter() by default): https://github.com/Starcounter/Starcounter.Startup/blob/master/Starcounter.Startup/Routing/Middleware/MasterPageMiddleware.cs#L29.
  3. As i can understand you suggest to add InteractionScope and middleware similar to DbScopeMiddleware that will work using following logic:
  • None => return next()
  • Create only => return new Transaction().Scope(next)
  • Attach only => return Db.Scope(next)
  • Create | Attach => return something from two points above depending on Transaction.Current != null?

(I decided to clarify this without sending the code because the changes doesn't seems to be very big.)

What do you think, should we return to the manage of the transaction scoping outside the MasterPageMiddleware (with invocation by default in AddRouter()) and create(change existing DbScopeMiddleware) separate middleware for this?

I did not participate in discussions on the latest changes in this library, so it’s difficult for me to be sure of any of the solutions.

@mtornwall
Copy link

  • None => return next()
  • Create only => return new Transaction().Scope(next)
  • Attach only => return Db.Scope(next)
  • Create | Attach => return something from two points above depending on Transaction.Current != null?

Not quite what I had in mind, but maybe we can simplify it to:

  • AttachOrCreate => return Db.Scope(next)
  • AlwaysCreate => return new Transaction(...).Scope(next)

WDYT?

@kegor
Copy link
Contributor Author

kegor commented Mar 18, 2019

@mtornwall, yeah, sound good for me, thank you for clarification.
The last (but not least) question here is about adding (changing existing DbScopeMiddleware) for using this new attribute. Do you think that we should return to this approach (instead of current case with manage of the transaction scoping in the MasterPageMiddleware)?

@kegor kegor self-assigned this Mar 18, 2019
@mtornwall
Copy link

Do you think that we should return to this approach (instead of current case with manage of the transaction scoping in the MasterPageMiddleware)?

As I understand it the transaction scope management in MasterPageMiddleware is only about whether or not the master page itself should have a scope attached. And so it doesn't affect how transaction scopes are assigned to attached view models. But this is based on the docs only, so I may be totally wrong.

Anyway, what I think we should have is:

  • An attribute called InteractionScope, since this is the terminology we want to use in the future (i.e. avoid the word transaction).
  • An InteractionScopeMiddleware (might just be a matter of renaming DbScopeMiddleware) that manages scope attachment.

WDYT?

@kegor
Copy link
Contributor Author

kegor commented Mar 18, 2019

@mtornwall, alright i agree with your guess regarding MasterPageMiddleware, and i think we can't achieve that without next 2 points. So i'll try to make some changes to check how it works.

@kegor
Copy link
Contributor Author

kegor commented Mar 29, 2019

We were discussing the last step which is consists of testing the newly released 0.8.0 version (https://www.nuget.org/packages/Starcounter.Startup/0.8.0) in all apps and updating them then and @mtornwall suggested to assign this action to him.

@kegor kegor assigned mtornwall and unassigned kegor Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request important
Projects
None yet
Development

No branches or pull requests

3 participants