Qluxzz / avanza

A Python library for the unofficial Avanza API
https://qluxzz.github.io/avanza/
MIT License
85 stars 40 forks source link

Pydantic models aren't used even though it's hinted as a return type #101

Open lov3b opened 1 month ago

lov3b commented 1 month ago

Well, the pydantic models which are defined and hinted as returntypes aren't actually instanciated. Instead most endpoints simply return the result of self__call which is either of dict or list. We can redefine the call-function to something like the following to serialize to a model. A problem which arises is that the test won't work with it's caching out of the box. Another problem, which arises are that some models aren't correctly implemented. For example the SearchResults model.

new __call method

P = TypeVar('P', bound=BaseModel)

   def __call(
            self, method: HttpMethod, path: str, options=None, return_content: bool = False,
            pydantic_model: Optional[Type[P]] = None
    ) -> Union[P, list, dict, bytes, None]:
        method_call = {
            HttpMethod.GET: self._session.get,
            HttpMethod.POST: self._session.post,
            HttpMethod.PUT: self._session.put,
            HttpMethod.DELETE: self._session.delete,
        }.get(method)

        if method_call is None:
            raise ValueError(f"Unknown method type {method}")

        data = {}
        if method == HttpMethod.GET:
            data["params"] = options
        else:
            data["json"] = options

        response = method_call(
            f"{BASE_URL}{path}",
            headers={
                "X-AuthenticationSession": self._authentication_session,
                "X-SecurityToken": self._security_token,
            },
            **data,
        )

        response.raise_for_status()

        # Some routes like add/remove instrument from a watch list
        # only returns 200 OK with no further data about if the operation succeeded
        if len(response.content) == 0:
            return None

        if return_content:
            return response.content

        if pydantic_model is None:
            return response.json()
        return pydantic_model.model_validate_json(response.content)

Broken SearchResults

class SearchHit(BaseModel):
    changePercent: float
    currency: str
    """ ISO 4217 """
    lastPrice: float
    flagCode: str
    """ ISO 3166-1 alpha-2 """
    tradable: bool
    tickerSymbol: str
    name: str
    id: str

class SearchResult(BaseModel):
    instrumentType: str
    numberOfHits: int
    topHits: List[SearchHit]

class SearchResults(BaseModel):
    totalNumberOfHits: int
Qluxzz commented 1 month ago

I'm not sure if we always want to validate the response, since if Avanza changes their API it will require an update to be published for the specific function to work again. If the user doesn't care about the specific field that is removed/updated they can still continue to use the specific function without any changes.

The typing is mostly just to help you discover what data you can get from a function.

But Pydantic might have a soft validation mode where that would work like that as well?

Qluxzz commented 1 month ago

But Pydantic might have a soft validation mode where that would work like that as well?

This seems to be the Pydantic way of doing just that:

Overview.model_construct(_fields_set=Overview.model_fields_set, **overview)
lov3b commented 1 month ago

Otherwise we could make use of TypedDict if the goal isn't to verify/assert the data, but to give hints to the user.

wenrir commented 3 weeks ago

Hello,

Sorry to hijack this issue but as it is recent and relevant please let me comment here rather than opening a new issue.

I have not contributed to this project and might not have the all the relevant background so please consider that my comment is subjective.

I'm not sure if we always want to validate the response, since if Avanza changes their API it will require an update to be published for the specific function to work again. If the user doesn't care about the specific field that is removed/updated they can still continue to use the specific function without any changes.

I think it's preferable to keep the __call function generic for this purpose and keep it this way. However,

The typing is mostly just to help you discover what data you can get from a function.

Doesn't this sort of destroy the purpose of type hinting? @lov3b makes a good point with introducing a more generic type.

If we see the following type hinted function, wouldn't we assume that the return value is Overview ?

    def get_overview(self) -> Overview:
        """Get account and category overviews"""
        return self.__call(HttpMethod.GET, Route.CATEGORIZED_ACCOUNTS.value)

but,

overview = avanza.get_overview()
# This will fail because overview is a dict not Overview
buying_power = overview.accountsSummary.buyingPower.value

In order to keep __call generic, maybe we can introduce a decorator to wrap the functions that utilizes __call to parse the model from response?

Qluxzz commented 2 weeks ago

The search result models have been fixed in commit 1f5573b.

But the issue of not instancing the Pydantic models correctly remains.