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

[BugFix] Return FMP Error Messages #6237

Merged
merged 21 commits into from
Mar 21, 2024
Merged

Conversation

deeleeramone
Copy link
Contributor

@deeleeramone deeleeramone commented Mar 19, 2024

WIP

  1. Why?:

    • This PR addresses a bunch of instances where FMP returns empty and does not relay the error message.
      • Invalid API keys were returning empty instead of the error message.
      • Where multiple items were allowed, many are not checking that each symbol was being returned.
  2. What?:

    • Refactor the HTTP requests to use methods that don't suppress the errors.
  3. Impact:

    • Renovates a couple of standard models that are too FMP-specific and all mandatory fields.
    • Error messages from FMP are no longer suppressed.
    • FMPAnalystEstimates now accepts multiple symbols.
  4. Testing Done:

    • Manual with bad key and good key, multiple items and single.
    • Not every endpoint raises a ClientResponseError, some will be empty and others are a dictionary with "Error Message" or "error" existing.

The examples below highlight the response difference between two, different, FMP endpoints with an invalid key.

obb.equity.ownership.insider_trading("AAPL")
OpenBBError: 401, message='Unauthorized', url=URL('https://financialmodelingprep.com/api/v4/insider-trading?symbol=AAPL&limit=500&page=0&apikey=********')
obb.equity.fundamental.cash("AAPL")
OpenBBError: FMP Error Message -> Invalid API KEY. Feel free to create a Free API Key or visit https://site.financialmodelingprep.com/faqs?search=why-is-my-api-key-invalid for more information.

@deeleeramone deeleeramone added bug Fix bug do not merge Label to prevent pull request merge platform OpenBB Platform v4 PRs for v4 labels Mar 19, 2024
@deeleeramone deeleeramone marked this pull request as ready for review March 19, 2024 19:31
@deeleeramone deeleeramone removed the do not merge Label to prevent pull request merge label Mar 19, 2024
Copy link
Contributor

@IgorWounds IgorWounds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeleeramone Is there a particular need to have most standard fields set as Optional? Wouldn't we want to guarantee a certain set of fields for a particular endpoint that would make sense to always be present?

The async def get_one(symbol) repeats quite a few times through the fetchers and we can make it a helper so that it can be easily imported. It could, for example, take the symbol and an URL as an optional field or the like.

@deeleeramone
Copy link
Contributor Author

deeleeramone commented Mar 20, 2024

The async def get_one(symbol) repeats quite a few times through the fetchers and we can make it a helper so that it can be easily imported. It could, for example, take the symbol and an URL as an optional field or the like.

They may appear to be repeating, and in the simplest cases it is, but there can be subtle differences and get_one is meant to be specific to the particular data that is being parsed - not universal. It is easier to maintain, and read the code, when it is all contained within the fetcher, IMO.

Is there a particular need to have most standard fields set as Optional? Wouldn't we want to guarantee a certain set of fields for a particular endpoint that would make sense to always be present.

The standard models should be usable for other providers that do not yet have this endpoint. In the past, I have wanted to add different providers to the earnings estimates but I could not because the model is only useful for FMP data. All those fields are quite likely to not be returned by another source.

@IgorWounds IgorWounds self-requested a review March 21, 2024 09:25
@IgorWounds IgorWounds added this pull request to the merge queue Mar 21, 2024
Merged via the queue into develop with commit 78695fa Mar 21, 2024
10 checks passed
@IgorWounds IgorWounds deleted the bugfix/fmp-error-messages branch March 21, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change bug Fix bug platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants