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

Integrating Database of Ab Initio Crystal Structures (DAICS) to Optimade #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zxwtstar
Copy link

Integrating Database of Ab Initio Crystal Structures (DAICS) to Optimade. Z. Allahyari

@merkys
Copy link
Member

merkys commented Apr 26, 2024

Looks legit. By the way, structure_features values do not seem to conform to OPTIMADE standard, it would probably be better to use custom prefixed field for them.

@ml-evs
Copy link
Member

ml-evs commented Aug 14, 2024

Hi @zxwtstar, just wondering if there's any progress on this? There still seems to be the issue around structure features that Andrius pointed out, plus the few comments I made over email. I am happy to merge this in the meantime to reserve the namespace, but we should probably turn off "aggregation" which will prevent this non-compliant data from being accessed in multi-provider queries, until the issues are fixed. (edit: closing the PR was accidental!)

@ml-evs ml-evs closed this Aug 14, 2024
@ml-evs ml-evs reopened this Aug 14, 2024
@zxwtstar
Copy link
Author

Dear @ml-evs I am sorry that this issue is not fixed yet. I tried to fix the problems that you mentioned once we had communicated by email, and I fixed issues in fact, but they did not appear on the database. And I couldn't fix that.
So, I got stuck for technical reasons and overloaded over time so... . I'll try to fix it again this week and if the problem persists, I appreciate it if you or your colleagues could help me solve it.

I also remember a bug, which I believe should be from the OPTIMADE side. The bug is:
Since DAICS data are available through API key generated through DAICS website for subscribed people. So, generally, people are directed through our server, and if they have a valid API key they can access the data. And everything works in this regard with our implementation.
The problem arises when users try to access data by replacing the domain or daics.net with its IP address. Then they can access all data without any required API key. Of course, we can close the corresponding ports in the firewall of our servers. But still, I was wondering if this can be solved from OPTIMADE side.

@ml-evs
Copy link
Member

ml-evs commented Sep 2, 2024

Dear @ml-evs I am sorry that this issue is not fixed yet. I tried to fix the problems that you mentioned once we had communicated by email, and I fixed issues in fact, but they did not appear on the database. And I couldn't fix that. So, I got stuck for technical reasons and overloaded over time so... . I'll try to fix it again this week and if the problem persists, I appreciate it if you or your colleagues could help me solve it.

No problem, there's no rush at all from our side, just didn't want you to think this was forgotten about from ours! Feel free to reach out over email if I can help with anything.

I also remember a bug, which I believe should be from the OPTIMADE side. The bug is: Since DAICS data are available through API key generated through DAICS website for subscribed people. So, generally, people are directed through our server, and if they have a valid API key they can access the data. And everything works in this regard with our implementation. The problem arises when users try to access data by replacing the domain or daics.net with its IP address. Then they can access all data without any required API key. Of course, we can close the corresponding ports in the firewall of our servers. But still, I was wondering if this can be solved from OPTIMADE side.

This sounds more like a server configuration issue than an OPTIMADE one. You would probably have to write some custom middleware to intercept requests and require an API key (we could add this generally too, I guess, but this will be quite dependent to your setup and how you issue keys).

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