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

Should global scopes of pivot model be applied? #261

Closed
radmen opened this issue Dec 12, 2017 · 10 comments
Closed

Should global scopes of pivot model be applied? #261

radmen opened this issue Dec 12, 2017 · 10 comments
Assignees

Comments

@radmen
Copy link
Contributor

radmen commented Dec 12, 2017

Here's quite interesting case.

First I'll try to give valid example code (based on docs):

App/Models/User

class User extends Model {
    ownedCars () {
        return this.belongsToMany('App/Models/Car')
            .pivotModel('App/Models/OwnerCar')
    }
}

App/Models/OwnerCar

class OwnerCar extends Model {
    static boot () {
        super.boot()
        this.addHook('beforeCreate', (userCar) => {
            userCar.is_current_owner = true
        })
        this.addGlobalScope(
            (query) => {
                query.where({ is_current_owner: true })
            }
        )
    }
}

Attaching car to user (using ownedCars()) will set properly is_current_owner flag.

After few hours of hard debugging I've found out that global scope is applied when using attach(), detach() yet it's not the case when I'm trying to use eg fetch() or count(). Same goes for eager loads (with('ownedCars') or load('ownedCars'))

I've dwelled into the code and it looks that global scope is not applied to data which is loaded eagerly.

Should global scope be applied when fetching data?
One thing I from the code I can tell is that it won't be that easy to implement.

@thetutlage
Copy link
Member

Seems like a bug by looking at first glance, lemme get into it in more depth later

@thetutlage
Copy link
Member

@radmen Mind testing same by installing lucid from Github?

@radmen
Copy link
Contributor Author

radmen commented Dec 24, 2017

@thetutlage is this update released on NPM? I've checked version 4.1.1 and the bug still occurs.

@thetutlage
Copy link
Member

Nope, that's why I said try it from Github

@thetutlage thetutlage reopened this Dec 24, 2017
@radmen
Copy link
Contributor Author

radmen commented Dec 25, 2017

Ah, sorry. Will check it now.

@radmen
Copy link
Contributor Author

radmen commented Dec 25, 2017

Fix still fails in my project.

Later (hopefully in next days) I'll prepare a clean Adonis installation and will try to reproduce it here.

@radmen
Copy link
Contributor Author

radmen commented Jan 3, 2018

@thetutlage I've prepared an example app which allows to reproduce the problem: https://github.com/radmen/adonis-lucid-pivot-global-scopes

@thetutlage
Copy link
Member

Looked at the repo, the global scope should be added to the Car model, since all the fetch queries are made using this model, the pivotModel is used only for attach, detach and sync calls.

@tlgreg
Copy link

tlgreg commented Jan 3, 2018

I might be misunderstanding something.
How would you do that?
The constraint is on the pivot and the scope is only make sense through the relation.

@thetutlage
Copy link
Member

Yes I believe the relationship should apply scopes from the pivot model when defined. Thinking of other relations too 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants