domjudge
domjudge copied to clipboard
DOMjudge
Use DTO's for POST/PUT request bodies.
Some notes:
- We need the Symfony built in serializer for request mapping, so that is why I'm adding it. We can't get rid of the JMS one, since we use virtualproperty and type annotations, which the Symfony one doesn't support.
- For adding submissions it's a bit ugly, since we have both the CLICS format and our own, so all fields are optional. Also our format uses file uploads, which the Symfony request mapper doesn't support.
- I haven't done anything to the judgehost controller, but should we? We can add way more typing there this way as well.
- All file uploads (logo's, etc) have also not been done.
- The contest patch call can't use the assert\callback to check for values, since there CLICS has a distinction between not passing a variable and setting it to
null. A bit ugly. - Some tests needed to be updated since the error messages are a bit different. Not a big issue I think.
- If you supply no body, the Symfony request mapper produces an empty 400 error. Also not a big issue I think.
After this we should discuss what more to change to DTO's @vmcj, you have gone through most code.
After this we should discuss what more to change to DTO's @vmcj, you have gone through most code.
The first candidates would be the *TwigData arrays, collect*Table arrays and the *Stats arrays, the DOMJudgeService file also has a lot of ugly arrays
The other individual functions:
- getConfigSpecification
- findExecutableOptions
- dataForEditor
- getStats
Some notes:
* We need the Symfony built in serializer for request mapping, so that is why I'm adding it. We can't get rid of the JMS one, since we use virtualproperty and type annotations, which the Symfony one doesn't support.
What is the new way of working with them? Is one the superset of the other or do they overlap?
* I haven't done anything to the judgehost controller, but should we? We can add way more typing there this way as well.
I would say so? It would make it much easier for people to create an IOI judge or if we plan to change the judgedaemon to another implementation?
* All file uploads (logo's, etc) have also not been done. * The contest patch call can't use the assert\callback to check for values, since there CLICS has a distinction between not passing a variable and setting it to `null`. A bit ugly.
Can we change the CLICS spec or is CLICS correct here?
* Some tests needed to be updated since the error messages are a bit different. Not a big issue I think.
Agree
* If you supply no body, the Symfony request mapper produces an empty 400 error. Also not a big issue I think.
That makes sense right, don't repeat that request if we can't do anything with it.
Some notes:
* We need the Symfony built in serializer for request mapping, so that is why I'm adding it. We can't get rid of the JMS one, since we use virtualproperty and type annotations, which the Symfony one doesn't support.What is the new way of working with them? Is one the superset of the other or do they overlap?
Nope they have different skillsets... Just use the JMS stuff like we used to and for request params the Symfony one will be used. Hopefully in the future the Symfony one is more nature and we can replace the JMS one.
* I haven't done anything to the judgehost controller, but should we? We can add way more typing there this way as well.I would say so? It would make it much easier for people to create an IOI judge or if we plan to change the judgedaemon to another implementation?
Well the API itself won't change right, only the parsing logic. But I agree, it might be better.
* All file uploads (logo's, etc) have also not been done. * The contest patch call can't use the assert\callback to check for values, since there CLICS has a distinction between not passing a variable and setting it to `null`. A bit ugly.Can we change the CLICS spec or is CLICS correct here?
I don't think we can easily change the CLICS spec. This is just a drawback of the serializer: there is no easy way to distinguish between null and that a value has not been set. What I can think of is to use an actual setter (and not a constructor argument) and then keep track there if it is actually called, but not sure if that's better.
* Some tests needed to be updated since the error messages are a bit different. Not a big issue I think.Agree
* If you supply no body, the Symfony request mapper produces an empty 400 error. Also not a big issue I think.That makes sense right, don't repeat that request if we can't do anything with it.