Conversation
…ce code duplication
…the root node of the list
Holmes98
left a comment
There was a problem hiding this comment.
For stripping comments, could we just do something like this?
to_xml do |xml|
xml.tag! 'statement', Loofah.fragment(statement).scrub!(Loofah::Scrubbers::NoComment.new)
endAlso, I think :owner_id should be hidden for problems, problem sets, contests, and groups (at least for non-admins).
| XmlUtil.serialize_id_list xml, 'contestants', contestants | ||
| end | ||
|
|
||
| if policy.scoreboard? then |
There was a problem hiding this comment.
| if policy.scoreboard? then | |
| if policy.show_details? then |
Problem IDs should be hidden to users unless the contest has ended or they are a current/past contestant (we may want to reuse problems). We should probably also restrict access to :problem_set_id in the same way.
| end | ||
|
|
||
| def to_xml(opts={}) | ||
| opts[:exclude] ||= [:startcode] |
There was a problem hiding this comment.
| opts[:exclude] ||= [:startcode] | |
| opts[:except] ||= [:startcode] |
| XmlUtil.serialize_id_list xml, 'contests', contests | ||
| XmlUtil.serialize_id_list xml, 'groups', groups |
There was a problem hiding this comment.
Same as with Problem#to_xml, should we restrict these to admins?
| XmlUtil.serialize_id_list xml, 'contests', contests | ||
| XmlUtil.serialize_id_list xml, 'groups', groups | ||
| XmlUtil.serialize_id_list xml, 'problem-sets', problem_sets |
There was a problem hiding this comment.
Should we restrict these to admins? This will list upcoming/current contests and groups/problem sets that the user doesn't have access to.
| # `expired_at` has the value Float::INFINITY when the request hasn't expired, | ||
| # and the XML formatter explodes when it encounters that value. Adding :expired_at | ||
| # to opts[:exclude] is the obvious solution, but for reasons unknown to me it does | ||
| # not appear to work as expected. Changing `expired_at` to some other value (which isn't | ||
| # a datetime) will result in an empty tag being emitted with a `nil="true"` attribute | ||
| # which seems like the best solution after just omitting the tag entirely.` |
| @@ -1,3 +1,5 @@ | |||
| require 'builder' | |||
There was a problem hiding this comment.
Doesn't seem to be used?
| require 'builder' |
Exposes additional API endpoints (e.g.
GET /problems/:id.xml), adds additional data to some endpoints (e.g.<groups>node when retrieving aProblemSet), and hides some sensitive info such as the judge log when retrieving submissions.Serializers
Serialization logic has been implemented by overriding the
to_xmlmethod of the relevant models which avoids the need to repeat:onlyand:includeall over the place. Ideally we'd want this logic separated out into serializer classes, but I couldn't figure out an elegant way of doing that.Some things I tried:
active_model_serializersgem does not support XML, and while the authors are interested in adding support they're only looking into it for version 0.10 which requires Rails 6. Train runs on Rails 4 and I imagine bumping by 2 major versions will break things (see: Added XML support rails-api/active_model_serializers#448 (comment))ActiveModel::Serializers::XMLis, afaict, built-in to Rails 4 (it was split out in v5?) and so isn't very helpfulBuilder::MarkupXmland wind up with strange extraneous elements inserted)serializable_hashmethod feels a bit nicer to override thanto_xml, but doesn't receive all of the builder options (:includeis missing, for instance)attributesmethod results in pretty strange XML, see thetotal-weightingelement here: https://i.imgur.com/w0zsVTj.pngIf there's a better place for the serialization logic or a library you know of, I'm happy to rework the PR. Ruby isn't my area of expertise :)
The stock
to_xmlbehavior addstypeattributes to nodes, but if you manually add a tag through a builder you don't get that attribute.XmlUtilexists mainly to ergonomically addtypeattributes to custom nodes in order to maintain consistency with the rest of the XML doc.Swapping over to JSON could be an idea, as it would let us use
active_model_serializers. JSON also handles whitespace perfectly, whereas with XML it's possible to configure your parser to strip out extraneous whitepsace -- which is a problem when working with submissions. This felt like a pretty major change though so I didn't do it.Didn't do
A few things mentioned on Discord haven't been done:
updated-at:updated-atworks great as a cache key and I can't see any issue with exposing it -- so it's still visible.nameonUserdocs; I've set it up so that if you hit the API from a staff account you get the user's name (and email), but otherwise those fields don't exist. This mirrors the behavior of the HTML page.Future
Here are some endpoints which I haven't included in this PR, but would like to have done eventually (eventually probably meaning never -- parsing the HTML feels easier than writing the serializers):
GET /contests/:id.xml. There are a lot of moving parts to that code and I'm not entirely sure I understand all the acl rules.GET /submissions/:id.xmlas most of the info is available through the web interface anyway, but I didn't do that either as I think there's room for discussion about the exact shape of the XML and it's also fairly straightforward to parse out judge results from the HTML, anyway. Here's where I got up to before deciding it was getting too big: https://i.imgur.com/ibclHnr.pngXML Samples
Here are some sample XML docs (note that
problemsin the contest sample is collapsed in the UI; the conteststartcodeis now censored, andpoints+max-pointsare also excluded from submissions now): https://imgur.com/a/UfJs203