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

Improve Payload CMS data fetching implementation #29

Merged
merged 5 commits into from
Jan 9, 2025

Conversation

m453h
Copy link

@m453h m453h commented Jan 8, 2025

This pull request addresses an issue encountered when accessing content from the Payload CMS API deployed in production.

It was observed that requests with forwarded HTTP headers from the web application were being redirected to a 404 page, which pointed to the charter.africa site. This appears to be an unrelated issue that requires further investigation.
However, discarding (not forwarding) the HTTP request headers from the client (React Application) when making a request to Payload CMS via the Flask backend resolves this issue.

Minor improvements have been implemented by introducing a unified function to handle all Payload CMS API requests and error handling. Furthermore, the existing Flask view functions have been refactored to make use of this function.

@m453h m453h self-assigned this Jan 8, 2025
@m453h m453h added the bug Something isn't working label Jan 8, 2025
server/views/cms/documents.py Fixed Show fixed Hide fixed
server/views/cms/documents.py Fixed Show fixed Hide fixed
server/views/cms/documents.py Fixed Show fixed Hide fixed
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@m453h m453h requested review from thepsalmist and kilemensi January 8, 2025 12:29
@m453h m453h marked this pull request as ready for review January 8, 2025 12:29
Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

👍🏽

It was observed that requests with forwarded HTTP headers from the web application were being redirected to a 404 page, which pointed to the charter.africa site. This appears to be an unrelated issue that requires further investigation.
However, discarding (not forwarding) the HTTP request headers from the client (React Application) when making a request to Payload CMS via the Flask backend resolves this issue.

Can we include an example dump of headers that cause redirect/errors?

Comment on lines 36 to 61
try:
headers = {
'Authorization': f'users API-Key {API_KEY}',
'Content-Type': 'application/json'
}

if application_name:
headers['Cs-App'] = application_name

escaped_args = {k: html.escape(v) for k, v in (params or {}).items()}

response = requests.get(url, params=escaped_args, headers=headers)
response.raise_for_status()
return response.json(), response.status_code

except requests.RequestException as e:
logger.error(f'Request failed: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500

except ValueError as e:
logger.error(f'Invalid JSON response: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500

except Exception as e:
logger.error(f'Unexpected error: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
try:
headers = {
'Authorization': f'users API-Key {API_KEY}',
'Content-Type': 'application/json'
}
if application_name:
headers['Cs-App'] = application_name
escaped_args = {k: html.escape(v) for k, v in (params or {}).items()}
response = requests.get(url, params=escaped_args, headers=headers)
response.raise_for_status()
return response.json(), response.status_code
except requests.RequestException as e:
logger.error(f'Request failed: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500
except ValueError as e:
logger.error(f'Invalid JSON response: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500
except Exception as e:
logger.error(f'Unexpected error: {str(e)}')
return jsonify({'message': 'An internal error has occurred.'}), 500
headers = {
'Authorization': f'users API-Key {API_KEY}',
'Content-Type': 'application/json'
}
if application_name:
headers['Cs-App'] = application_name
try:
escaped_args = {k: html.escape(v) for k, v in (params or {}).items()}
response = requests.get(url, params=escaped_args, headers=headers)
return response.json(), response.status_code
except requests.RequestException:
logger.exception('Request failed')
except ValueError:
logger.exception('Invalid JSON response')
except Exception:
logger.exception('Unexpected error')
return jsonify({'message': 'An internal error has occurred.'}), 500
  1. In a try/except, logger.exception includes e by default (unlike logger.error).
  2. Do we need response.raise_for_status()?
  3. Reduce empty lines

Copy link
Author

Choose a reason for hiding this comment

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

Cool, made the changes

Regarding 2. response.raise_for_status():
I think we we do need it, the motive was to throw a RequestException for the 404 response that is coming with HTML content (from charter.africa)...but I'm also open to let it throw an Invalid JSON response

Copy link
Member

Choose a reason for hiding this comment

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

I think we we do need it, the motive was to throw a RequestException for

Cool, cool... we can leave it there for better logs I guess.

@m453h
Copy link
Author

m453h commented Jan 8, 2025

👍🏽

It was observed that requests with forwarded HTTP headers from the web application were being redirected to a 404 page, which pointed to the charter.africa site. This appears to be an unrelated issue that requires further investigation.
However, discarding (not forwarding) the HTTP request headers from the client (React Application) when making a request to Payload CMS via the Flask backend resolves this issue.

Can we include an example dump of headers that cause redirect/errors?

Sure here are the dumps, I did more tests on this, it appears that the Host header is the one that is causing the issue

{
   "User-Agent":"PostmanRuntime/7.43.0",
   "Accept":"*/*",
   "Postman-Token":"*****",
   "Host":"localhost:8000",
   "Accept-Encoding":"gzip, deflate, br",
   "Connection":"keep-alive",
   "Cookie":"lng=en"
}
{
   "Host":"localhost:8000",
   "User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:133.0) Gecko/20100101 Firefox/133.0",
   "Accept":"*/*",
   "Accept-Language":"en-US,en;q=0.5",
   "Accept-Encoding":"gzip, deflate, br, zstd",
   "Referer":"http://localhost:8000/",
   "Connection":"keep-alive",
   "Cookie":"lng=en;",
   "Sec-Fetch-Dest":"empty",
   "Sec-Fetch-Mode":"cors",
   "Sec-Fetch-Site":"same-origin",
   "Priority":"u=4"
}

@kilemensi
Copy link
Member

kilemensi commented Jan 8, 2025

Thanks @m453h. Based on the definition of host header, doesn't it sound like we need to change it? As in frontend will set host to Python APIs. The Python APIs need to "forward" this request to the actual final PayloadCMS & hence they must change host to the IP/domain of PayloadCMS.

... anyways, we can pick this up tomorrow.

@m453h
Copy link
Author

m453h commented Jan 9, 2025

Thanks @m453h. Based on the definition of host header, doesn't it sound like we need to change it? As in frontend will set host to Python APIs. The Python APIs need to "forward" this request to the actual final PayloadCMS & hence they must change host to the IP/domain of PayloadCMS.

... anyways, we can pick this up tomorrow.

Yes that's correct 💯, I made a slight change by just removing the header, and let Python requests set it, it does fix the issue

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

LGTM!

@m453h m453h merged commit 721ebd9 into main Jan 9, 2025
2 checks passed
@m453h m453h deleted the fix/cms-request-rejection branch January 9, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants