-
Notifications
You must be signed in to change notification settings - Fork 4
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 include filtering #40
Conversation
Trying to perform
I realise that this is probably due to this going through a many-to-many relationship rather than a one-to-one or one-to-many/many-to-one, but this is an example of a query that needs to be able to be done. |
@louise-davies Is that request meant to return the same as [
{
"ID": "1",
"CREATE_ID": "simple/root",
"CREATE_TIME": "2019-05-21 11:24:46",
"MOD_ID": "simple/root",
"MOD_TIME": "2019-05-21 11:24:46",
"INSTRUMENT_ID": "11",
"INVESTIGATION_ID": "1",
"INVESTIGATION": {
"ID": "1",
"CREATE_ID": "simple/root",
"CREATE_TIME": "2019-05-21 11:24:46",
"DOI": "None",
"ENDDATE": "2017-07-21 12:01:54",
"MOD_ID": "simple/root",
"MOD_TIME": "2019-05-21 11:24:46",
"NAME": "Proposal 1",
"RELEASEDATE": "None",
"STARTDATE": "2017-07-13 12:01:54",
"SUMMARY": "None",
"TITLE": "quas accusantium omnis",
"VISIT_ID": "Proposal 1 - 1",
"FACILITY_ID": "1",
"TYPE_ID": "16"
},
"INSTRUMENT": {
"ID": "11",
"CREATE_ID": "simple/root",
"CREATE_TIME": "2019-05-21 11:24:41",
"DESCRIPTION": "None",
"FULLNAME": "Instrument 19",
"MOD_ID": "simple/root",
"MOD_TIME": "2019-05-21 11:24:41",
"NAME": "I19",
"TYPE": "None",
"URL": "None",
"FACILITY_ID": "1"
}
}, Did that request work previously on master? |
I haven't played about with the include filters that much so I don't know if it worked on master - my thoughts are probably not, and If you mean |
@louise-davies Ok, I will have a look. Ah I posted the wrong endpoint I meant That is what gives the result I posted previously. Which nests investigation and instrument into InvestigationInstrument |
@keiranjprice101 yeah that does work, but that means that I either don't get the Investigation information in a way I can filter it, or I have to fetch Investigations and then send more individual requests to find out the instruments. Another problem is that users can currently sort/filter by Instrument in both Diamond and ISIS - which means we need to be able to sort/filter on an included entity... |
@louise-davies Ok. I think this should be ok but, filtering on includes is very different to normal filtering due to the way the Filters are defined in their classes. I've had to extend filtering to get the first endpoint in #34 working and I believe it would be a similar process. I will open a separate issue for this. |
@keiranjprice101 yeah, feel free to split them up into separate issues. Are you going to try and get many-to-many includes working in this PR or in a separate issue? aka should I approve this as is or should I wait? |
@louise-davies If everything else is fine with this, then you could approve it. It seems to me that, #42, many to many includes, #38, #39 and possibly #41 are all very very similar in what changes/code they will need. So I think I will have to put a lot of thought into changes that will play nicely between these issues. So I think this PR would start as a good base at least. Unless of course I am missing something obvious and these issues aren't as complicated as I believe. |
closes #32 closes #24
I made an error when staging these changes so the commits got mixed up, but it's small enough to just look at files changed instead.
Include filtering now allows any arbitrary combination of includes. However still only one filter per request (#39)
Examples:
http://localhost:5000/datafiles?where={"ID":3}&include={"DATASET":"INVESTIGATION"}
http://localhost:5000/datafiles?where={"ID":3}&include=["DATAFILEFORMAT", {"DATASET":{"INVESTIGATION":["INVESTIGATIONTYPE","FACILITY"]}}]
http://localhost:5000/datafiles?where={"ID":3}&include=["DATASET","DATAFILEFORMAT"]
http://localhost:5000/datafiles?where={"ID":3}&include=["DATASET"]
e.g
All of these return properly and this PR will make #38 easier once merged in