R7 json api #74

dexw25 · 2017-12-03T03:49:59Z

Resolves #38.

Implemented more complete read only JSON api. There is capability to read a full list or single selection from bridge crews, members, and missions. Ranks have also been separated out under /api/rank/<rank_type>/<member_id> to avoid calculating them unless the information is explicitly requested.

There is capability through this to read every static element of the datastore, except for member QR codes (they wouldn't format easily to text). Every endpoint requires a valid apiKey, and the lists support semester-based filtering.

ZMYaro · 2018-02-01T02:09:16Z

api.py

 		'name': member.name,
 		'dce': member.dce,
-		'mailingList': member.mailing_list,
-		'currentStudent': member.current_student,
+		'mailing_list': member.mailing_list,


Because this function is formatting a member to be returned as JSON, I think camelCase should be used to align with the JavaScript standard.

You make a good point. I have made the case consistent

ZMYaro · 2018-02-01T02:10:07Z

api.py

@@ -37,24 +86,26 @@ def get(self):
 		selected_semester = self.request.get('semester')

 		if selected_semester:
-			members = Member.query(Member.show == True, Member.semesters_paid == selected_semester).order(Member.name).fetch(limit=None)
+			# Avoid throwing error 500 if a bad semester string is supplied


Very minor, but since the file needs edits anyway, would you mind terribly if I requested period at the end of the sentence?

ZMYaro · 2018-02-01T02:14:34Z

api.py

 		else:
-			members = Member.query(Member.show == True, Member.never_paid == False).order(Member.name).fetch(limit=None)
+			members = Member.query().order(Member.name).fetch(limit=None)


This appears to expose members who asked to not appear on the website (the purpose of the Member.show == True), as well as non-members in the system beacuse they ran a mission or are on the mailing list (Member.never_paid == False).

I thought that it would be simpler to provide access to all members through the API, and do the member filtering in the app that pulls data, since there could be cases where we might want access to data for members that have not paid or don't wish to be seen publicly. Since access to the API is gated by the API key anyway, better to give more information here and avoid limiting uses of this interface later on down the road.

I am pretty sure the database has more non-members than members (between the mailing list and non-members who ran missions). Fetching every member for every API call is going to make a lot of queries unnecessarily slow and put a lot of unnecessary load on the server.

What do you think about adding a URL parameter for including non-members in the system that is False by default?

Sure, makes sense. I will implement a default False parameter to include private or non-paid members, that way we can only pull the whole list when necessary

ZMYaro · 2018-02-01T02:15:07Z

api.py

-		output = []
-		for member in members:
-			output.append(format_member(member))
+		output = [format_member(member) for member in members]


Good idea; much cleaner! 👍

Gotta love python list comprehensions

ZMYaro · 2018-02-01T02:17:14Z

api.py


 		self.response.headers['Content-Type'] = 'application/json'
 		self.response.write(json.dumps(output))

 class MemberAPI(webapp2.RequestHandler):
-	def get(self):
+	def get(self, id):


Why the move from URL parameter to part of the path?

It is a lot cleaner to be part of the path instead of a URL parameter, makes the URL structure more readable, and also allows the ability to select only specific parts of the member object if desired. For instance, for performance reasons it may be desirable someday to just pull /members/<id>/semestersPaid for a large set of members, then this class can just have another optional parameter added with a case for that.

When would you use /members/<id>/semesterspaid though? The handling of that API call would be identical to the existing member API, except with format_member stripping out the extra fields. Unless something changed since I last read through the GAE datastore docs, all datastore queries are the equivalent of SELECT * FROM..., so the load on the datastore would presumably be the same.

On the other hand, /members?id=<id> makes it clear that /members is the API URL.

Makes sense, code will be simpler if the members endpoint just handles everything. I will strike out MemberAPI and just add handling for id as a parameter to /members, and make it just return a list of one member if filtered by id

Also it's probably gonna be a lot less confusing going forward to remove the distinction between member and members as endpoints (IMO)

ZMYaro · 2018-02-01T02:20:15Z

api.py

@@ -71,14 +122,131 @@ def get(self):
 		self.response.headers['Content-Type'] = 'application/json'
 		self.response.write(json.dumps(output))

+class RankAPI(webapp2.RequestHandler):
+	def get(self, rank_type, id):


Why parts of the path rather than parameters?

See my other comment on URL path structure, I think I will remove the rank_type parameter though, since per your other comment that is probably not the best way to handle that interface.

See my reply above about URL path structure.

ZMYaro · 2018-02-01T02:22:52Z

api.py

+				self.error(400)
+				return
+
+		if rank_type == 'name':


Instead of returning rank types, what about returning a Rank object, like how ranks are defined in ranks.py, that just includes all three? You already set the output type as JSON, so it does not need to just be one string.

Good point. Changed to return an object with all 3 options rather than selecting one

ZMYaro · 2018-02-02T16:23:16Z

api.py

+		output = {
+		'rankName': member.get_rank_name(selected_semester),
+		'rankDisp': member.get_rank_disp(selected_semester),
+		'rankWithName': member.get_name_with_rank(selected_semester)}


Since the API is to fetch the rank, not the member, I am inclined to call the rank fields, name, abbr, and disp (for consistency with ranks.py), and then if we want to include name-with-rank, give that a name that suggests the member name is being appended to the rank, not the other way around.

Agree, will rename to be more consistent. Is rankName good for signifying we are appending the name?

Also I realized looking through this right now that there is no method to access abbr in ranks.py, is it worth adding in this commit?

ZMYaro · 2018-02-02T16:23:43Z

api.py

+
+		# Offer option to filter by a semester, but by default just send all missions
+		if selected_semester:
+			# Avoid throwing error 500 if a bad semester string is supplied


Sentence ending punctuation (sorry)?

Will correct (sorry I am bad at formatting comments)

ZMYaro · 2018-02-02T16:29:01Z

api.py

 		else:
-			members = Member.query(Member.show == True, Member.never_paid == False).order(Member.name).fetch(limit=None)
+			members = Member.query().order(Member.name).fetch(limit=None)


I am pretty sure the database has more non-members than members (between the mailing list and non-members who ran missions). Fetching every member for every API call is going to make a lot of queries unnecessarily slow and put a lot of unnecessary load on the server.

What do you think about adding a URL parameter for including non-members in the system that is False by default?

dexw25 · 2018-03-27T03:12:58Z

I rebased to master (again) and addressed comments. please review again and let me know if it's good
@ZMYaro
@noblelady

ZMYaro · 2018-04-05T23:19:50Z

api.py

+		'youtubeUrl': mission.youtube_url
+	}
+
+def format_bridgecrew(crew):


“Bridge Crew” is two words, so this should be “format_bridge_crew”.

ZMYaro · 2018-04-05T23:20:28Z

api.py

+	def get(self):
+		if not check_authentication(self):
+			return
+		bridgecrews = BridgeCrew.query().fetch(limit=None)


Same as above—“bridge_crews”.

ZMYaro · 2018-04-05T23:28:16Z

api.py

+	('/api/missions/?', MissionListAPI),
+	('/api/mission/([a-z0-9]+)', MissionAPI),
+	('/api/bridgecrews/?', BridgeCrewListAPI),
+	('/api/bridgecrew/([a-z0-9]+)?', BridgeCrewAPI),


Either /rank should not accept A-Z, or /mission and /bridgecrew should accept. I am inclined to say the latter so the error comes from it being an invalid ID requested rather than appearing to be an invalid API endpoint.

But then might it be better to just make those each (.+) (or ([^/]+)) and have the functions handle the error states?

ZMYaro · 2018-04-05T23:29:44Z

models.py

+
+	def get_mission_ids(self):
+		missions = Mission.query(Mission.runners == self.id).order(Mission.start).fetch(limit=None)
+		return [mission.id for mission in missions]


What is the benefit of this function existing? I think it would make more sense to do the list comprehension in the one place you use Member.mission_ids.

I am also concerned that the get_mission_ids function existing might suggest to someone new to the codebase that get_mission_ids is somehow less resource- or time-intensive than get_missions.

dexw25 · 2018-06-08T19:17:28Z

rebased and addressed comments @ZMYaro please re-review when you can

… add rank API

…r to get mission IDs run

…ation of semester logic. Also basic semester validation, avoid error 500 if the parameter cannot be cast to a float

…unless semester filter is applied

…ess explicitly specified otherwise

…an up comments

…name bridge_crew local variable. MODELS: Remove extraneous datastore property mission_ids and associated method

dexw25 added the new function label Dec 3, 2017

dexw25 added this to the r7 milestone Dec 3, 2017

dexw25 requested review from ZMYaro and noblelady December 3, 2017 03:50

ZMYaro requested changes Feb 1, 2018

View reviewed changes

dexw25 force-pushed the r7-json-api branch from de5930f to b20b2a6 Compare February 2, 2018 15:00

ZMYaro reviewed Feb 2, 2018

View reviewed changes

dexw25 force-pushed the r7-json-api branch from 37eadbb to 918988d Compare March 27, 2018 02:59

ZMYaro requested changes Apr 5, 2018

View reviewed changes

dexw25 force-pushed the r7-json-api branch 2 times, most recently from f876cce to 77517a2 Compare June 8, 2018 19:16

dexw25 force-pushed the r7-json-api branch from 77517a2 to b7d2d91 Compare June 8, 2018 19:23

dexw25 added 12 commits June 8, 2018 15:27

Fully flesh out member api, remove lingering APIKeyHandler reference,…

7835503

… add rank API

Cleanup member api formatting, add mission api, add property to membe…

9c72ce3

…r to get mission IDs run

Convert semester parameters in API requests to floats for proper oper…

661fcbc

…ation of semester logic. Also basic semester validation, avoid error 500 if the parameter cannot be cast to a float

Add bridge crew API. Fix API request format. Consistency edits in api.py

b9ee901

Change member list to show all members regardless of payment status, …

f41cff6

…unless semester filter is applied

Make JSON formatted data use camelCase consistently for field names

0fe2235

change rank api to return a rank object

898d6ad

add period to end of sentence in a comment

a8258f2

Add parameter all to member list api to filter to public member unl…

98d286f

…ess explicitly specified otherwise

Remove MemberAPI and incorporate functionality into MemberListAPI

191b1d1

Rename rankAPI fields to be more consistent with other files, and cle…

d5c7232

…an up comments

API: make URL matching more consistent, rename format_bridge_crew, re…

b7d2d91

…name bridge_crew local variable. MODELS: Remove extraneous datastore property mission_ids and associated method

dexw25 closed this Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R7 json api #74

R7 json api #74

dexw25 commented Dec 3, 2017

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 1, 2018

dexw25 Mar 27, 2018

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 2, 2018

dexw25 Mar 27, 2018

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 2, 2018 •

edited

Loading

dexw25 Mar 27, 2018 •

edited

Loading

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 2, 2018

ZMYaro Feb 1, 2018

dexw25 Feb 2, 2018

ZMYaro Feb 2, 2018

dexw25 Mar 27, 2018

ZMYaro Feb 2, 2018

dexw25 Mar 27, 2018 •

edited

Loading

ZMYaro Feb 2, 2018

dexw25 commented Mar 27, 2018

ZMYaro Apr 5, 2018

ZMYaro Apr 5, 2018

ZMYaro Apr 5, 2018

ZMYaro Apr 5, 2018

ZMYaro Apr 5, 2018

dexw25 commented Jun 8, 2018

R7 json api #74

R7 json api #74

Conversation

dexw25 commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZMYaro Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

dexw25 Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexw25 Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexw25 commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexw25 commented Jun 8, 2018

ZMYaro Feb 2, 2018 •

edited

Loading

dexw25 Mar 27, 2018 •

edited

Loading

dexw25 Mar 27, 2018 •

edited

Loading