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

Fixing Dockerfile and refactoring #26

Merged
merged 5 commits into from
Aug 3, 2024
Merged

Fixing Dockerfile and refactoring #26

merged 5 commits into from
Aug 3, 2024

Conversation

kuefmz
Copy link
Collaborator

@kuefmz kuefmz commented Jul 2, 2024

No description provided.

JJ-Author

This comment was marked as outdated.

Copy link
Contributor

@JJ-Author JJ-Author left a comment

Choose a reason for hiding this comment

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

see comments please

@@ -80,28 +56,28 @@ def get_ontology_from_request(request):
if v[0].decode('utf-8') == 'Host':
print('host found')
host = v[1].decode('utf-8')
path = request.path.decode('utf-8')
ontology = 'https://' + host + request.path.decode('utf-8')
Copy link
Contributor

@JJ-Author JJ-Author Jul 2, 2024

Choose a reason for hiding this comment

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

this hardcoded https seems really unsafe to me.
is there really no better way to get the scheme (protocol) - e.g. from the request object or by passing it as parameter?
keep in mind that this empty host seems to be a bug and could be fixed then this would break
also note that path is decoded twice

@JJ-Author
Copy link
Contributor

we openend this issue here abhinavsingh/proxy.py#1437
for the problem with the request object.
also I opened this issue here #85
that monitors adressing of separating logic from implementation details as e.g. in the commented if condition.
these 2 issues demand later changes such that I now merge the PR to not further block it.

@JJ-Author JJ-Author merged commit 0225a34 into dbpedia:main Aug 3, 2024
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

Successfully merging this pull request may close these issues.

3 participants