params doesn't make sense if we don't have a request

[jpmelos]

If we don't have an active request context, evaluating params doesn't make sense and is analogous to the attribute not existing at all, so we raise an AttributeError to make hasattr return False.

[jpmelos]

Tests are failing because of an unrelated problem.

[tsx]

Should we change the lookup mechanism instead? What if someone had a raw_data on their object which is also a property on Resource.

[jpmelos]

@tsx what do you suggest as an alternative for the lookup mechanism? I think the simplest solution here doesn't feel wrong, and it would be to also raise AttributeError from raw_data, with the same rationale: it simply doesn't make sense to request raw_data when there's no request context active.

[jpmelos]

Another alternative is to blacklist some attribute names for documents based on what attributes a default resource has. Blacklist params, raw_data and others (didn't check if there are others and what they would be). The rationale here is simply a naming conflict: since these names have a use other than fetch attributes to be serialized, using these as attributes that could be serialized could potentially bring problems.

For example, in the future an unknowingly developer could want to overwrite the params attribute from OpportunityExport by creating a params method in the resource, and with this override the params property from the base class and potentially seeing weird side-effects later when the code tries to reach for params with the original intention.

In a sense, the blacklist strategy feels more future-proof. But simply raising AttributeError from both params and raw_data, since they return non-callables and don't make sense when there's no request context active, is simpler and backwards-compatible.

@tsx do you have an opinion on this?

[tsx]

For example, in the future an unknowingly developer could want to overwrite the params attribute from OpportunityExport by creating a params method in the resource, and with this override the params property from the base class and potentially seeing weird side-effects later when the code tries to reach for params with the original intention.

Yes this is an issue with current design in general. I'd suggest separating resource's own processing stuff from these "magic accessors". But obviously this breaking design change is way out of scope of current fix.

But simply raising AttributeError from both params and raw_data is not very future proof. Imagine next person wanting to add some other new property. It's unlikely that they'll know to add this special case check in that new property. Same goes for the blacklist - they just won't add it.

My alternative (it might be a little controversial because of more introspection involved) is to check the resource's class (not resource instance) for the existence of the attribute (you'll get just the property descriptor so it won't try to execute it), and do that in a way that doesn't include superclasses - so we can ignore anything coming from a base Resource.

At the end of the day though, raise AttributeError is a viable fix if you need to just move on as long as you document why is so. I'll leave the decision up to you.

[jpmelos]

I tried a proof-of-concept of a more introspective solution like the one you described, and I think it becomes complex really quickly for not that much benefit over the simpler solution of raising AttributeError wherever it makes sense.

The one I tried is to go for the class's __dict__, this will make it clear whether a resource class has an attribute without considering superclasses. But, for example, OpportunityExportResource has this ancestry:

Resource (in flask-mongorest)
BaseResource (in closeio)
OrganizationBaseResource
RestrictedQueryOrganizationBaseResource
ExportResource
OpportunityExportResource

So, maybe I should evaluate also the __dict__ from ExportResource? That makes sense to me? But should I stop there, or look for the attribute in a few more levels, or all the way until just before Resource?

I think the tradeoff between complexity, maintainability and readability here makes me prefer the simplest implementation.

Even if there's a simpler introspective solution than the one I tried, we can always improve things later by revisiting this topic at a later time.