search

mapping-commons / sssom-api

Apache License 2.0
7 stars 0 forks source link

Functional get mappings by CURIE endpoint #5

Closed anitacaron closed 2 years ago

pkalita-lbl commented 2 years ago

Overall this looks like a nice start! I just have two comments:

  1. I'm a little curious about the SparqImpl class in src/database/sparql_implementation.py. Maybe there are future plans for it that I'm failing to grasp, but it seems like a bit of an unneeded abstraction on top of its superclass, SparqlImplementation. Since SparqImpl doesn't hold any state, I could easily imagine the work being done in get_sssom_mappings_by_curie moved to the get_mappings function and have get_mappings accept a SparqlImplementation instance instead. I wonder if perhaps the hesitation to do it that way was because of accessing the "private" members _sparql_values and _parse. It might be a conversation to have with the OAK development team about why those are underscored.
  2. For checking the format of CURIE query parameters, you might have a look at FastAPI's Query and its regex parameter: https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#add-regular-expressions. Query can be used either with or without Depends (Depends is helpful if many endpoints will need the same parameter validation, otherwise directly using Query in the response handler parameters is probably fine). Using Query might give better error response handling out-of-the-box.
pkalita-lbl commented 2 years ago

Big oversight on my part: I didn't realize at first that get_sssom_mappings_by_curie is already a method on SparqlImplementation (actually via its superclass AbstractSparqlImplementation). It looks like the implementation here is adding some additional where clauses to the query. I wonder if that's functionality that would be useful to have in the OAK library core. I'll take a look at it and if it makes sense to do that this can be simplified considerably. But don't let that hold up these changes in the meantime.

anitacaron commented 2 years ago

Thank you for the comments, Patrick!

  1. The idea is to replace the get_sssom_mappings_by_curie in AbstractSparqlImplementation! The query is written differently from the RDF we get from the sssom-py. This method is related to SSSOM but does not use the RDF structure expected/generated by sssom-py lib. Also, we might need to add an OPTIONAL attribute to the SparqlQuery class to retrieve all optional fields from SSSOM.

  2. I'm going to check FastAPI's Query. It makes sense to use it in this case. Thanks!