I843 CLICS API 2023-06 Endpoint Compliance #864

johnbrvc · 2023-11-16T23:10:42Z

Description of what the PR does

Implements version 2023-06 of the CLICS API specification endpoint services.
Creates a mechanism for adding newer (and older) versions of the CLICS specification relatively easily by creating a package for each CLICS version.
Adds PC2 account roles to endpoint service access.

Issue which the PR addresses

Partial work towards Issue #843 and these issues: #630 #697 #704 #709 #710 #712 #720 #787 #537 #562 #837 #840 #1016 #1048

Environment in which the PR was developed (OS,IDE, Java version, etc.)

Windows 11
Java 1.8.581

Helpful information for reviewing the PR

The majority of the files related to this PR are in the src/edu/csus/ecs/pc2/clics/ folder.
Specifically, the majority of those files are in the src/edu/csus/ecs/pc2/clics/API202306 folder (package).
The files in the src/edu/csus/pc2/clics/API202003 folder (package) have been relocated from the src/edu/csus/ecs/pc2/web folder (package). The idea is that each API version will have its own package under the src/edu/csus/ecs/pc2/clics folder. Nomenclature can be changed if folks do not like it as is.
Files in other folders (core, model, imports/ccs) contain either bug fixes or additional methods or changes to methods to support the new CLICS API Java package structure.
The src/edu/csus/ecs/pc2/util/JSONTool.java file has had many of its utility methods changed to public static (since they do not depend on an object instance), and, the new CLICS API Java package structure allows the methods to be shared easily without instantiating an object. Maybe JSONTool methods should be moved to JSONUtilities ?
Files in the services folders (core, eventFeed, web, ui) were changed to support selection of different API versions from the Event Feed thick client GUI.

Precise steps for testing the PR (i.e., how to demonstrate that it works correctly)

Right now, it's lots of manual web page queries. The following endpoints should work and return information compliant with 2023-06. Other endpoints will return whatever we used to return. (Note: Replace the contestId below, Default-5639045423021663843 with whatever contest ID you are testing with).

Of course, you have to run an event-feed client, and select 2023-06 as the API version, then start the web services.
Selecting 2020-03 will make the web services behave like they always did in the past (whatever API version that was).

You can change the links below to something like:
https://judge2:judge2@localhostt:50443/contests/Default-5639045423021663843/problems
or
https://team8:team8@localhostt:50443/contests/Default-5639045423021663843/problems
or
https://admin:admin@localhostt:50443/contests/Default-5639045423021663843/problems
to test out how the endpoints behave for various roles. The first 2 above use accounts from the accounts list configured into PC2. The last one uses a role from the realms.properties file. Both types of login information are accepted. Obviously, only PC2 judges, teams and administrator accounts are added to the realm as acceptable login ID's.

https://localhost:50443/
https://localhost:50443/contests
https://localhost:50443/contests/Default-5639045423021663843
https://localhost:50443/contests/Default-5639045423021663843/access
https://localhost:50443/contests/Default-5639045423021663843/account
https://localhost:50443/contests/Default-5639045423021663843/accounts
https://localhost:50443/contests/Default-5639045423021663843/accounts/AccountID
https://localhost:50443/contests/Default-5639045423021663843/teams
https://localhost:50443/contests/Default-5639045423021663843/teams/TEAMID
https://localhost:50443/contests/Default-5639045423021663843/problems
https://localhost:50443/contests/Default-5639045423021663843/problems/PROBLEMID
https://localhost:50443/contests/Default-5639045423021663843/judgement-types
https://localhost:50443/contests/Default-5639045423021663843/judgement-types/JUDGEMENTTYPE
https://localhost:50443/contests/Default-5639045423021663843/clarifications
https://localhost:50443/contests/Default-5639045423021663843/clarifications/CLARID
https://localhost:50443/contests/Default-5639045423021663843/groups
https://localhost:50443/contests/Default-5639045423021663843/groups/GROUPID
https://localhost:50443/contests/Default-5639045423021663843/organizations
https://localhost:50443/contests/Default-5639045423021663843/organizations/ORGID
https://localhost:50443/contests/Default-5639045423021663843/judgements
https://localhost:50443/contests/Default-5639045423021663843/judgements/JUDGEMENTID
https://localhost:50443/contests/Default-5639045423021663843/runs
https://localhost:50443/contests/Default-5639045423021663843/runs/TESTCASEID
https://localhost:50443/contests/Default-5639045423021663843/submissions
https://localhost:50443/contests/Default-5639045423021663843/submissions/RUNID
https://localhost:50443/contests/Default-5639045423021663843/submissions/TESTCASEID/files
https://localhost:50443/contests/Default-5639045423021663843/languages
https://localhost:50443/contests/Default-5639045423021663843/languages/LANGID
https://localhost:50443/contests/Default-5639045423021663843/state
https://localhost:50443/contests/Default-5639045423021663843/awards
https://localhost:50443/contests/Default-5639045423021663843/scoreboard
https://localhost:50443/contests/Default-5639045423021663843/scoreboard?group_id=GROUPID
https://localhost:50443/contests/Default-5639045423021663843/scoreboard?division=# (*)

(*) - The division query parameter is a PC2 extension and is NOT part of the CLICS API specification. It is not prohibited by the specification to add additional query parameters. Since PC2 supports the notion of divisions, eg groups with "D1", "D2", etc. in their names, the division query parameter allows filtering of the scoreboard by division.

PATCH requests work for contests/contestid/ endpoints as per the spec. Certain things, like countdown_pause_time are parsed and validated, but the back end code to implement them in PC2 is not present yet. eg. stop the countdown clock and display the countdown pause time, resume countdown, etc.

POST requests work for contests/contestid/clarifications endpoint to add new clarifications. PUT is not implemented and there are no plans to do so since it requires reworking object ID's (PUT allows a user to specify the internal ID of a clarification)

POST/PUT/PATCH are not implemented for awards endpoint. A NON_IMPLEMENTED error is returned to the client.

The access endpoint only returns properties for those services that are implemented for 2023-06 so far (listed above).

The following endpoint services are not applicable to a CCS and will NOT be implemented:

persons
commentary

troy2914

code review, left some comments.

realm.properties

src/edu/csus/ecs/pc2/clics/API202003/ResourceConfig202003.java

src/edu/csus/ecs/pc2/clics/API202306/CLICSClarification.java

src/edu/csus/ecs/pc2/clics/API202306/CLICSProvider.java

src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java

src/edu/csus/ecs/pc2/clics/API202306/ProblemService.java

src/edu/csus/ecs/pc2/clics/API202306/SubmissionService.java

src/edu/csus/ecs/pc2/services/core/JSONUtilities.java

src/edu/csus/ecs/pc2/services/eventFeed/WebServer.java

src/edu/csus/ecs/pc2/services/web/ICLICSResourceConfig.java

cacerts.pc2

troy2914

seems this was submitted in error.

troy2914

add new statements to one in samps too.

pc2v9.ini

src/edu/csus/ecs/pc2/clics/API202306/ScoreboardService.java

troy2914

reviewed new changes. seems good, other than a old hack.

src/edu/csus/ecs/pc2/clics/API202306/CLICSAccount.java

troy2914

still have a few outstanding comments.

troy2914 · 2023-12-15T20:33:12Z

btw, you have to expand the expansions on github to see all the comments that I have left.

troy2914

These latest commits look normal to me.

troy2914 · 2024-01-29T15:40:56Z

hmm, just looking at the commits, if seems you pull'ed some changes from develop, instead of rebase develop.

johnbrvc · 2024-02-03T01:15:55Z

still have a few outstanding comments.

I believe I have addressed all the things that require code changes.

pc2v9.ini

samps/pc2v9.ini

Added some comments and changed variable name to clarify freeze time calculation in CLICSContestState. Validate properties array for CLICSEndpoint.

clevengr

Re-reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSContestState.java; looks good to me.

src/edu/csus/ecs/pc2/clics/API202306/CLICSContestState.java

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java; left comments. Nothing huge that I couldn't live with...

clevengr · 2025-02-20T22:12:37Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+ * CLICS Organization
+ * Contains information about an Organization (institution)


Same comments as on other files, about JavaDoc.

Fixed in next push.

clevengr · 2025-02-20T22:15:44Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+     * Fill in organization information properties
+     * TODO: fix to use organization obtained from organizations.json.  From insitutions.tsv, we get this:
+     * [0]:1329 [1]:New York University [2]:NYU
+     * but only need [1].  We get the rest from the a typical account.


Not sure what ...from the a typical account is trying to say. And is it talking about a PC2 account?

Clarified the comment in next push.

clevengr · 2025-02-20T22:16:43Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+     * @param account typical account using this organization
+     * @param orgFields String array of fields that correspond to institutions.tsv.  Gack!


orgId is missing from the @param list...

Fixed in next push.

clevengr · 2025-02-20T22:19:08Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+     * [0]:1329 [1]:New York University [2]:NYU
+     * but only need [1].  We get the rest from the a typical account.
+     *
+     * @param account typical account using this organization


I'm not at all sure what typical account using this organization means. How does an account "use" an organization? (Does this mean, for example, the kind of account which would normally login on behalf of the organization? Or does it mean the kind of CCS-internal account that would somehow "manipulate" this organization?

I clarified the comment. Basically, it's an account that is associated with the organization. Several accounts can "use" the same organization, eg. multiple teams from the same school use the same Organization.

clevengr · 2025-02-20T22:22:05Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+        id = orgId;
+        icpc_id = orgId;


It seems weird that orgId is assigned to both these fields. If we don't have any notion of "organization" within PC2, then perhaps one of these should be NULL instead of set to the value of the other? (I'm not sure where this gets used, so I'm a bit stabbing in the dark; it just seems weird to me to assign the same thing to two different fields when those two fields clearly exist for some separate reasons...)

In all cases that I can think of, the id and icpc_id are the same, at least for DOMjudge and PC2. The id property can be anything a CCS wants. We (PC2) just happen to say we're going to use the CMS ID (which is the icpc_id) for the id. Perfectly legal.

clevengr · 2025-02-20T22:24:04Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+            ObjectMapper mapper = JSONUtilities.getObjectMapper();
+            return mapper.writeValueAsString(this);
+        } catch (Exception e) {
+            return "Error creating JSON for organization info " + e.getMessage();


Add "CLICS" to message?

Added in next push.

clevengr · 2025-02-20T22:25:05Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+            institutionsMap = createInstitutionsFromJSON(mapper.readValue(jsonfile, CLICSOrganization[].class), log);
+        } catch (Exception e) {
+            // deserialize exceptions
+            log.log(Log.WARNING, "could not deserialize organizations file " + jsonfile.toString(), e);


Add "CLICS" to log message?

Fixed in next push.

clevengr · 2025-02-20T22:26:05Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+            institutionsMap = createInstitutionsFromJSON(mapper.readValue(json, CLICSOrganization[].class), log);
+            // deserialize exceptions
+        } catch (Exception e) {
+            log.log(Log.WARNING, "could not deserialize organizations string", e);


Add "CLICS" to log message?

Fixed in next push.

clevengr · 2025-02-20T22:26:34Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+    }
+
+    /**
+     * Convert CLICS organizations to PC2 hashma


What's a "hashma" :)

Don't tell me you've never heard of a hashma ? It's the "mother" of all Hash Maps! (I changed it to the lesser, hashmap, in the comment for the next push).

clevengr · 2025-02-20T22:27:36Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSOrganization.java

+        for(CLICSOrganization org: corgs) {
+
+            if(StringUtilities.isEmpty(org.id)) {
+                log.log(Log.SEVERE, "no id property in organization");


Add "CLICS" to log message?

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSProblem.java; minor comments.

clevengr · 2025-02-20T22:31:14Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblem.java

+ * CLICS Problem
+ * Contains information about a Problem


Same comments about JavaDoc.

Yes. As I mention below, I will fix these comments on related classes in the next push.

clevengr · 2025-02-20T22:36:48Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblem.java

+            ObjectMapper mapper = JSONUtilities.getObjectMapper();
+            return mapper.writeValueAsString(this);
+        } catch (Exception e) {
+            return "Error creating JSON for problem info " + e.getMessage();


Add "CLICS" to error message. Also, shouldn't this be logged?

Added CLICS to message in next PR.

We will have to discuss at next week's meeting the general concept of logging every error that the CLICS API can generate for faulty data it receives. I'm not sure that's a good or useful idea.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSProvider.java; minor comments.

clevengr · 2025-02-20T22:43:32Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+ * CLICS Score (for a problem)
+ * Contains information about the score a team received for a single problem


Same comments about JavaDoc.

Also, since this class is the PC2 implementation of what the CLICS spec refers to as "Problem Data" (very poor name, I agree -- but that's what's in the spec), it would probably be useful to have a comment in this header that mentions the fact that this class corresponds to what in CLICS is called "Problem data"...

Fixed in next push.

clevengr · 2025-02-20T22:50:45Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProvider.java

+     * @param versionInfo
+     */
+    public CLICSProvider(VersionInfo versionInfo) {
+        name = "pc2";


Should this perhaps be upper case ("PC2")? Seems a bit more formal...

Sure. Changed in next push.

clevengr

I attempted to review src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java. However, I ran into so many illogical things that I suspended the review. See my comments for details. I look forward to something that tells me whether the issue is with the code or with my review process...

clevengr · 2025-02-20T22:56:11Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java

+ * CLICS Judgment Run
+ * Contains information about a Run


Same comment about JavaDoc. Also, I'm not sure what the meaning of Judgment Run in the first line is.

Also, I think it would be helpful (for future developers) to add a note here about the difference between the term "Run" in CLICS (execution of a single test case) and "Run" in PC2 (a submission from a team).

The CLICSRun class is for the "Judgements" API endpoint.. I thought "Judgements" was too similar to "Judgement Types", so I decided to call the class CLICSRun instead of CLICSJudgement to avoid confusion with CLICSJudgementType. So, we have a CLICSSubmission, which is the contestant submission and the CLICSRun which is the disposition of a Submission (Accepted, Wrong, RTE, etc.)

Now, there is also something in CLICS called "Runs", which PC2 calls "Test Cases", and I called the CLICS class CLICSTestCase.

I will update the comments at the top of each of these modules to indicate how each class corresponds to its CLICS endpoint name in the next push.

I think naming the class CLICSRun is REALLY confusing. We already have two notions of "run" in the system: a PC2 "run", which is a submission from a team, and a CLICS "run", which is the execution of a single test case. That's confusing enough by itself; to add a THIRD notion of "run" which is not the same as either one of the existing notions but instead is related to the judgement associated with a run (PC2) or the collection of judgements associated with a submission (CLICS) -- seems to me to add a LOT of unnecessary additional confusion.

Further, as you said above, what you've called CLICSRun is "the disposition of a Submission (Accepted, Wrong, RTE, etc.)". The phrase disposition of a Submission seems (to me) to clearly be something that's a Judgement -- that's what happens (that's what you "get back") when you send in a "Submission". So I don't really see why CLICSJudgement is that bad a name. (Yes, I agree that it could be [slightly] potentially confused with CLICSJudgementType, but I think the presence of Type in the latter name makes it clear that the latter class is about types of judgements, whereas a name like CLICSJudgement indicates an instance of a judgement (of some "type").

In any case, I think the negatives of adding yet another meaning of "run" considerably outweigh the (possible) slight confusion between "judgement" and "judgement_type". If you're really adamant that this shouldn't be called CLICSJudgement, I would at least suggest using something that avoids adding confusion by using the term "run". Perhaps something like CLICSSubmissionResult? In any case I really, really think we should avoid introducing yet another meaning for "run"; we've already got enough confusion in the overloaded use of that term.

At the time I wrote this last year, it seemed like a good name (CLICSRun). In hindsight, maybe not. I don't care either way, so I renamed it to CLICSJudgement to match the specification name for it. I was going to name it CLICSJudgment (which is the correct spelling for judgment), but I figured it should match the british spelling they chose to use in the CLICS API. In my comments, and my variables, I use the (correct) American English spelling though (judgment). I wish they weren't inconsistent in their language use in the spec. Ex. If they use "judgement" they should also use "colour", but, it's what we are stuck with.

While I am not actively going through and changing PC2 classes and variables to "judgment", when I make new variables or classes I am using "judgment". Whenever I see "judgement" in the code, I just grit my teeth and wonder why it was done that way. Fixed in next push.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java. I found a number problems, either with the code or with my understanding of what's going on. Looking forward to an explanation of one or the other.

clevengr · 2025-02-20T23:48:24Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+     * @param versionInfo
+     */
+    public CLICSProblemScore(HashMap<String, String> probEleToShort, ProblemSummaryInfo psi) {
+        num_judged = Utilities.nullSafeToInt(psi.getAttempts(), 0);


The call to psi.getAttempts() worries me. That "getter" returns the value of the private field attempts in the PSI. Since that field is private, the only way it can have a reasonable (i.e. non-zero) value is if some code somewhere calls PSI.setAttempts(). However, there's not a single reference to PSI.setAttempts() anywhere in the entire PC2 project. So how does the value returned by getAttempts() represent the value which should be stored innum_judged?

It also bothers me that the field attempts in PSI is a string... but that would be workable if someone ever assigned a value to attempts.

ProblemSummaryInfo is an javax.xml type object. It is used to deserialize XML. Recall the DSA (and NSA) both create an XML representation of the scoreboard. This class deserializes a piece of that XML scoreboard into an object for use in such things as, well, this. As such, the setter methods will not be called when it is used to deserialize XML, but, the fields do get set.

clevengr · 2025-02-20T23:50:00Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+     */
+    public CLICSProblemScore(HashMap<String, String> probEleToShort, ProblemSummaryInfo psi) {
+        num_judged = Utilities.nullSafeToInt(psi.getAttempts(), 0);
+        num_pending = Utilities.nullSafeToInt(psi.getIsPending(), 0);


Same issue with the call to getIsPending(); as far as I can tell no code anywhere actually sets that value;

clevengr · 2025-02-20T23:51:41Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+    public CLICSProblemScore(HashMap<String, String> probEleToShort, ProblemSummaryInfo psi) {
+        num_judged = Utilities.nullSafeToInt(psi.getAttempts(), 0);
+        num_pending = Utilities.nullSafeToInt(psi.getIsPending(), 0);
+        problem_id = psi.getProblemId();


Same issue with the call to `getProblemId(); as far as I can tell no code anywhere actually sets that value.

clevengr · 2025-02-20T23:52:52Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+        if(probEleToShort.containsKey(problem_id)) {
+            problem_id = probEleToShort.get(problem_id);
+        }
+        solved = toBool(psi.getIsSolved(), false);


Same issue with the call to `getIsSolved(); as far as I can tell no code anywhere actually sets that value.

clevengr · 2025-02-20T23:54:04Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSProblemScore.java

+        solved = toBool(psi.getIsSolved(), false);
+        if(solved) {
+            // Problem solution time is in minutes, so multiply by 60 seconds.
+            time = ContestTime.formatTime(StringUtilities.getIntegerValue(psi.getSolutionTime(), 0)*60);


Why the multiplication by 60? The CLICS spec says time is the number of MINUTES into the contest when the problem was solved...

Yes. That is wrong for CLICS API 2023-06, however, it was right for the draft spec since it's a RELTIME in the draft (HH:MM:SS). I guess I was being a bit ahead of myself here. I reverted it back to plain old minutes, and changed the data type as well. Next commit.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java. Request strong consideration for changing the class name, along with a potential issue regarding assigning a value to the id property (see comments).

clevengr · 2025-02-21T06:26:20Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java

+    private double max_run_time;
+
+    /**
+     * Fill in properties for a Problem description.


This comment doesn't make sense; this class isn't dealing with Problem properties.

Yeah, another cut/paste error. It's good you're finding these. I fix them as I see them, but some fall through the cracks. Fixed in next push.

clevengr · 2025-02-21T06:27:18Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java

+     * @param model The contest
+     * @param problem The problem


These @param values don't correspond to the actual arguments in the constructor (and those actual arguments are missing from the comments).

Copy cut/paste. Fixed in next push.

clevengr · 2025-02-21T06:44:35Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java

+    public CLICSRun(IInternalContest model, IInternalController controller, Run submission, Set<String> exceptProps) {
+        // {"id":"189549","submission_id":"wf2017-32163123xz3132yy","judgement_type_id":"CE","start_time":"2014-06-25T11:22:48.427+01",
+        // "start_contest_time":"1:22:48.427","end_time":"2014-06-25T11:23:32.481+01","end_contest_time":"1:23:32.481"}
+        id = submission.getElementId().toString();


The id property of this class (assigned by the above statement) is (according to the CLICS spec) supposed to be the id of the judgement, not the submission. As written, the id is getting assigned the ElementId of the submission. I think the id needs to be assigned by (something like) the following pseudo-code:

state = submission.getState(); if (state == Run.RunStates.JUDGED) { //the run has been judged, so get the active judgement record jr = submission.getJudgementRecord(); id = jr.getElementId(); } else { // i haven't thought about what to do here (the case where the Run hasn't been judged); // -- I think it's "throw an exception", or at least log a severe error, because there should never be // a case where this method is called with a Run that hasn't been judged... }

This way the "id" is the id of the PC2 "judgement record", not the submission id. (It seems clear that it should not be the "submission id", because otherwise what is the separate submission_id field for?)

Now what I haven't thought deeply about is, what's the relationship between a PC2 JudgementRecord and a CLICS "judgement object". But it seems clear to me that, at least, the value assigned to id should NOT be the submission_id (which is what the existing code does...)

I'm also not at all sure that we want to be exposing PC2 "ElementIds" as externally-viewable ids (but I haven't thought that through thoroughly...) Would it make more sense to call submission.getNumber() and use THAT as the id?

Some background: I used the same values for "id" and "submission_id" that we have been using for years in the previous spec version (2020-03) - see: convertJudgementToJSON in API202003/JSONTool.java. In an of itself, this is not a valid reason for not changing it, however ... read on.... there IS a reason.

The "submission_id" property IS the submission number. And, that is the ID that is used for CLICSSubmission records (you'll get to that when you review CLICSSubmission). Since the judgementRecord is tied directly to the submission, I suppose the original author figured it made sense to use the submission's unique ID, which I think is reasonable. It also makes looking up the judgment a bit easier for the "\<contestid\>/judgements/\<ID\>" API endpoint.

In addition, if the submission is not yet "judged", (it could be "queued for judgment", "new", or "being judged"), we still have to furnish a "bare-bones"
judgment record, and since PC2 does not HAVE a judgment record yet, we still need a (judgment) id, so the submission's elementId is a reasonable choice, IMHO. In fact, the spec says, that a judgment record may not have a judgement_type_id if there is no "verdict" (and I HATE that word) yet. Basically, judgement_type_id is only "required iff [the] judgement has completed." If there is no "verdict" yet, many of the properties in the (now) CLICSJudgement record will not be serialized in the JSON (the spec has them as ?)

Unless there are strong objections, I'm inclined to leave it "as-is", not only because it has worked for years, but because I don't see a viable alternative for the non-judged case.

Refactor CLICSRun -> CLICSJudgement, and all references. Various comment and log message clarifications. Fixed bug in CLICSProblemScore with returning the time as RELTIME instead of int (minutes).

The YAML parser will convert any value it sees with a fractional part (decimal point followed by numbers) to a double value, which is converted to a String in ContestSnakeYAMLLoader. The causes a problem when that value was parsed as a Long (it caused a NFE). This fix converts the value to a double using (Double.parseDouble()), then, it typecasts that to a long. This is OK, since contest durations are in seconds, not fractional parts of second. We just ignore the fractional part. Also fixed JUnit test correctly since the freeze duration will now compare properly without stripping off the leading 0. This fix will be part of the PR for i843_CLICS_API_Endpoint_Compliance, but can be cherry-picked elsewhere if needed before this is merged.

Do not compare for leading 0 on freeze duration anymore.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSRun.java. Request strong consideration for changing the class name, along with a potential issue regarding assigning a value to the id property (see comments).

clevengr · 2025-02-21T21:05:59Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScore.java

+ * CLICS team score
+ * Contains information the score for a team


Same comment about JavaDoc... Also, the second line isn't really a complete sentence...

Fixed in next push.

src/edu/csus/ecs/pc2/clics/API202306/CLICSScore.java

clevengr · 2025-02-22T00:47:04Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScore.java

+        total_time = Utilities.nullSafeToInt(teamStanding.getPoints(), 0);
+        if(num_solved > 0) {
+            // Problem solution time is in minutes, so multiply by 60 seconds.
+            time = ContestTime.formatTime(StringUtilities.getIntegerValue(teamStanding.getLastSolved(), 0)*60);


I noted previously another file where "multiplication by 60" was inappropriate because the expected result was to be in minutes. In this case the CLICS spec doesn't explicitly say in this case what the time units are (it did say "minutes" in the other case I noted), but I would think it should be minutes here as well -- both for consistency (well, I guess that would depend on whether we think the CLICS spec is, or is intended to be, consistent :) as well as that PC2 (and other WF CCS's) apply penalty minutes.

Also, as noted previously, the default value of "0" for getIntegerValue() means that a value of null or empty returned from getLastSolved() means that a team gets a default time of zero, which is an advantage to the team; I'm not sure that's what we should be assigning in that case.

This was the same problem as I had in the other module where I was "ahead of the curve" in implementing the draft spec and not 2023-06. The draft wants a RELTIME here, which is HH:MM:SS, so you'd need the time in seconds. 2023-06 still has it as minutes. The format of time is fixed in the next push. Note that time is a String, as is the value returned by getLastSolved(), so if it's null it will not be included in the serialized JSON, so that point is moot about a "default value". Fixed in next push.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java. Made several comments, none of which are showstoppers.

clevengr · 2025-02-22T00:52:57Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

+ * CLICS Scoreboard
+ * Contains information about the scoreboard


clevengr · 2025-02-22T01:07:29Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

+        state = new CLICSContestState(model, null);
+
+        ArrayList<CLICSScoreboardRow>rowsArray = new ArrayList<CLICSScoreboardRow>();
+        HashMap<String, String> probEleToShort = new HashMap<String, String>();


When I read the name of this variable (and I've seen it in other classes as well), I immediately interpreted it as representing a HashMap which maps problem elementIds (probEle) to (java type) short. In fact, it's mapping ElementIds to problem short names. For clarity, I would suggest naming the variable (something like) probEleToShortName.

Since you think it's confusing, I changed it. Doesn't bother me either way. Changed in next push.

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

clevengr · 2025-02-22T01:13:23Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

+        // list of xml entries for each team's standing
+        List<TeamStanding> standings = contestStandings.getTeamStandings();
+        if(standings != null) {
+            for (TeamStanding teamStanding : contestStandings.getTeamStandings()) {


Why is contestStandings.getTeamStandings() called here again, when the code two lines earlier created variable standings which contains that value?

Because it was a mistake. Just uses standings now. Next push it's fixed.

clevengr · 2025-02-22T01:17:28Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

+        if(standings != null) {
+            for (TeamStanding teamStanding : contestStandings.getTeamStandings()) {
+              rowsArray.add(new CLICSScoreboardRow(probEleToShort, teamStanding));
+              rows = rowsArray.toArray(new CLICSScoreboardRow[0]);


I don't understand why rowsArray is being converted (and assigned to rows inside the for loop. It seems like this conversion should happen (once) after the for loop finishes...

Yeah, neither do I. I think I grabbed the essence of this code from somewhere else. (I forget where). And, it lost something in the translation to this module. I can tell it was "stolen" from somewhere else because the indenting was wrong too. (2 spaces instead of 4) Fixed in next push.

clevengr · 2025-02-22T01:18:24Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboard.java

+            ObjectMapper mapper = JSONUtilities.getObjectMapper();
+            return mapper.writeValueAsString(this);
+        } catch (Exception e) {
+            return "Error creating JSON for scoreboard info " + e.getMessage();


Add "CLICS" to message?

Added in next push.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboardRow.java. Several comments, no showstoppers.

clevengr · 2025-02-22T01:21:58Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboardRow.java

+ * CLICS Scoreboard row
+ * Contains information about a single entry in the scoreboard


Fixed in next push.

clevengr · 2025-02-22T01:24:14Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboardRow.java

+    /**
+     * Fill in API scoreboard row information properties (for scoreboard endpoint)
+     * 
+     * @param probEleToShort hashmap for mapping problem elementid to shortname


As noted elsewhere, I think probEletoShort is confusing (with respect to Java type short); I'd recommend changing the name to probEleToShortName.

Changed in next push.

src/edu/csus/ecs/pc2/clics/API202306/CLICSScoreboardRow.java

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java. Made several comments; no show-stoppers.

clevengr · 2025-02-22T01:30:35Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

+import edu.csus.ecs.pc2.services.core.JSONUtilities;
+
+/**
+ * CLICS Team object


Fixed in next push.

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

clevengr · 2025-02-22T01:49:32Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

+            ObjectMapper mapper = JSONUtilities.getObjectMapper();
+            return mapper.writeValueAsString(this);
+        } catch (Exception e) {
+            return "Error creating JSON for team info " + e.getMessage();


Add "CLICS" to error message?

Fixed in next push.

clevengr · 2025-02-22T01:52:55Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

+            ObjectMapper mapper = new ObjectMapper();
+            newaccounts = createTeamsFromTeamsJSON(contest, mapper.readValue(jsonfile, CLICSTeam[].class), site, institutionsMap, log);
+        } catch (Exception e) {
+            log.log(Log.WARNING, "could not deserialize team file " + jsonfile, e);


Add "CLICS" to message?

Fixed in next push.

clevengr · 2025-02-22T01:54:23Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

+            ObjectMapper mapper = new ObjectMapper();
+            newaccounts = createTeamsFromTeamsJSON(contest, mapper.readValue(json, CLICSTeam[].class), site, institutionsMap, log);
+        } catch (Exception e) {
+            log.log(Log.WARNING, "could not deserialize team string", e);


Add "CLICS" to message?

Fixed in next push.

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

clevengr · 2025-02-22T02:09:44Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSTeam.java

+                }
+            } catch(Exception e) {
+                // Some sort of conversion error - log it and abort
+                log.log(Log.SEVERE, "unable to get team number from label or id", e);


Suggest adding "CLICS" in front of "label" and "id".

Added in next push.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/CLICSVerionInfo.java. Request changing class name to fix typo.

clevengr · 2025-02-22T02:19:49Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSVerionInfo.java

+ * @author John Buck
+ *
+ */
+public class CLICSVerionInfo {


Name of class is misspelled (Verion -> Version).

Did you see who wrote this module originally - haha ?

* @author Douglas A. Lane <[email protected]> * @author John Buck

Doug certainly did this type of thing A LOT. :-)
Fixd in nxt psh.

clevengr · 2025-02-22T02:23:20Z

src/edu/csus/ecs/pc2/clics/API202306/CLICSVerionInfo.java

+            ObjectMapper mapper = JSONUtilities.getObjectMapper();
+            return mapper.writeValueAsString(this);
+        } catch (Exception e) {
+            return "Error creating JSON for version info " + e.getMessage();


Suggest adding "CLICS" to error message.

Fixed in next push.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java. Have some substantial concerns about whether the synchronize clause on the POST method is going to cause problems. Have suggested at least a partial workaround.

clevengr · 2025-02-22T02:26:45Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+     *
+     * @param sc security info for the user making the request
+     * @param contestId Contest for which info is requested
+     * @return a {@link Response} object containing the contest languages in JSON form


Copy/paste error? contest languages --> contest clarifications...

Yes. Fixed in next push.

clevengr · 2025-02-22T02:27:41Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+            return Response.status(Response.Status.NOT_FOUND).build();
+        }
+
+        // get the groups from the contest


groups --> clarifications ?

Yep. Fixed in next push.

clevengr · 2025-02-22T02:35:39Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+     * @param sc security info for the user making the request
+     * @param contestId Contest for which info is requested
+     * @param clarificationId the id of the desired clarification
+     * @return a {@link Response} object containing the contest languages in JSON form


This line doesn't describe what the method returns (it returns a single clarification, not a language).

clevengr · 2025-02-22T02:36:40Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+        if(contestId.equals(model.getContestIdentifier()) == false) {
+            return Response.status(Response.Status.NOT_FOUND).build();
+        }
+        // get the groups from the contest


Oops... copy/paste: groups --> clarifications.

Yep. Fixed in next push.

clevengr · 2025-02-22T02:38:02Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+        // be set to non-null
+        Clarification clarNoAnswer = null;
+
+        // create list of clarifications to send back


This comment is a bit misleading, because the method isn't supposed to return a LIST -- just ONE clarification.

Yes, I already fixed that before you found it. Didn't push it though. Will be pushed now. Also, made the comment a bit better indicating that it first checks for an answer, and if not found, it checks for the clarification itself.

clevengr · 2025-02-22T07:50:51Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+            return Response.status(Status.BAD_REQUEST).entity("must include time property").build();
+        }
+
+        controller.getLog().log(Level.WARNING, "Admin PUT clarification is not implemented due to forcing the ID");


I'm not sure what ...due to forcing the ID means. Could the message explain slightly more?

I think it's clear. We can't force ID's in PC2.

Well, I guess it still seems vague to me. Are you talking about the fact that PC2 can't support the ability of an external client to set the id of an existing clar?

clevengr · 2025-02-22T07:59:39Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+     *
+     * @param contestId contest identifier
+     * @param jsonRequestString
+     *            a JSON string specifying a starttime request in CLICS format


It's not clear to me why this method has anything do to specifically with a starttime request... Copy/Paste?

And actually, it's not clear that this method is ever used anywhere...

Deleted the method. Not used. Left over from copy. Next push.

clevengr · 2025-02-22T08:05:04Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+                }
+            }
+        }
+        return(false);


I'm pretty sure this statement is intended to be return(ret) ?

Yes, that's a bug. Fixed in next push.

clevengr · 2025-02-22T08:07:14Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+    /**
+     * Try to find the Clarification from its id
+     *
+     * @param szClarId This ID of the clar


The parameter name here (szClarId) doesn't make sense... nor does the word This...

Fixed in next push.

clevengr · 2025-02-22T08:08:52Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+
+        Clarification clar = null;
+
+        // create list of clarifications to send back


This comment doesn't quite make sense -- there is no "list of clarifications being sent back". I think it's supposed to say (something like) create list of clarfications to be checked ? Although in fact no "list" is actually created at all; it's just fetched from the model...

Fixed in next push.

Mostly fixes to comments to correct misspellings. Some nomenclature changes for variables. Fixed CLICSVersionInfo class name spelling error.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/ContestService.java. Mostly innocuous comments; no show-stoppers. I did express concern about two things marked as "TODOs" that we might need as Primary - so maybe we need to file Issues on them?

clevengr · 2025-02-22T18:28:41Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+        final Map<String, String> jsonDataMap;
+
+        try {
+            jsonDataMap = mapper.readValue(jsonRequestString, mapType);


Hmm... I would have thought that since jsonDataMap is declared final, it's not allowed to assign a value to it here. No?

(I do see that it is final but it hasn't been "initialized"; I assume that has something to do with it. But I've never seen that construct in Java. I assume it works?)

Maybe this sheds some light on it. Of course, local variables must be initialized before used, or you will get an error.

clevengr · 2025-02-22T18:30:43Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+
+        // if the map is null then the parsing failed
+        if (requestMap == null) {
+            controller.getLog().log(Log.WARNING, LOG_PREFIX + contestId + ": unable to parse JSON starttime string");


It doesn't appear to me that at this point we know that the request was a starttime patch -- only that it failed to parse correctly. (It appears to me that it could, for example, have been a (failed) scoreboard_thaw_time patch.)
If I'm right about that then the above message is misleading...

Yep. Fixed in next push.

clevengr · 2025-02-22T18:31:26Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+        if (requestMap == null) {
+            controller.getLog().log(Log.WARNING, LOG_PREFIX + contestId + ": unable to parse JSON starttime string");
+            // return HTTP 400 response code per CLICS spec
+            return Response.status(Status.BAD_REQUEST).entity("Bad JSON starttime request").build();


Same comment: how do we know this was a (failed) starttime PATCH (as opposed to some other (failed) PATCH)?

Yep. Fixed in next push.

clevengr · 2025-02-22T18:35:51Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+            return Response.status(Status.BAD_REQUEST).entity("Bad JSON starttime request").build();
+        }
+
+        // if we get here then the JSON parsed correctly; see if it contained "starttime" as a key


This comment is misleading; the following lines of code are checking for the existence of CONTEST_ID_KEY, not "starttime".

Yep. Fixed in next push.

clevengr · 2025-02-22T19:57:26Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+        // Flag to indicate that countdown pause time was specified
+        boolean sawCountdownPauseTime = false;
+
+        // if we get here then the JSON parsed correctly; see if it contained "start_time" as a key (that is required by spec)


Maybe I'm misinterpreting the spec, but it appears to me that start_time is NOT "required" (as stated by the above comment -- rather, a PATCH could instead consist of (just) id and scoreboard_thaw_time. If I'm right, that means the above comment isn't exactly correct... "start_time" isn't required. (Indeed, the following code appears to correctly check for scoreboard_thaw_time if start_time is missing... my point is that the comment explicitly (incorrectly) states that start_time is required.)

Yep. I clarified the test for CONTEST_THAW_TIME as well. Fixed in next push.

clevengr · 2025-02-22T21:39:45Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+        GregorianCalendar thawTime = getDate(contestId, thawTimeValue);
+        if (thawTime != null) {
+            // Set thaw time to this time.
+            // TODO: tell PC2 to thaw the contest at the given time.


THIS todo I think we're ok without for Primary. The two other ones (noted elsewhere), I worry we might need...

clevengr · 2025-02-22T21:45:23Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+     * This method returns a representation of the current contest scheduled start time in JSON format as described on the CLICS wiki.
+     *
+     * @return a {@link Response} object containing a JSON String giving the scheduled contest start time as a Unix Epoch value, or as the string "undefined" if no start time is currently scheduled.


These comments refer to "scheduled start time", but I don't see that this method has anything to do with that...

Fixed in next push.

clevengr · 2025-02-22T21:47:38Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+    @GET
+    @Produces(MediaType.APPLICATION_JSON)
+    public Response getContests() {
+        CLICSContestInfo [] allContests = new CLICSContestInfo[1];


Am I correct that this array is size[1] because, while we're pretending that we're implementing a full CLICS "multi-contest" API, in fact PC2 only handles a single contest?

Yes, I fixed the comment to explain that. Fixed in next push.

clevengr · 2025-02-22T21:49:39Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+    }
+
+    /**
+     * Check the user has a role than change contest start time


Weird syntax in this comment...

clevengr · 2025-02-22T21:50:25Z

src/edu/csus/ecs/pc2/clics/API202306/ContestService.java

+    }
+
+    /**
+     * Check the user has a role than change contest thaw time


Ditto - weird syntax in comment.

Oh my, yes. I have no idea where that wording came from. Gibberish. I also added the following TODO: to the comment:

/** * Check that the user's role permits setting contest thaw time * TODO: Should we always return false here since we don't implement this? * * @param sc Security context for the user * @return true if the user can perform the operation */

Fixed in next push.

clevengr

Resolved one comment.

src/edu/csus/ecs/pc2/clics/API202306/CLICSScore.java

clevengr

re-reviewed src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java. Resolved a number of comments; still have a couple that are awaiting a further response.

clevengr · 2025-02-23T00:01:05Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+                clarAnswers = clarification.getClarificationAnswers();
+                if (clarAnswers != null) {


Ok, I think this another case where I didn't explain myself properly/sufficiently. The code in question is:

clarAnswers = clarification.getClarificationAnswers(); if (clarAnswers != null) { for (ClarificationAnswer clarAns: clarAnswers) { if (clarAns.getElementId().toString().equals(clarificationId)) { try { //map the found clar into JSON and return it, or else return a JSON error... } ... } } }

I believe what this code is trying to do is:

get all answers to the specified clarification; if (there are some answers) { look through the answers to see if an answer matches the specified clarification; if a match is found, return it;

If I've misunderstood the intent of the code as described above, never mind. But if I'm correct, then my point is that the test if (there are some answers) is implemented in the code as if (clarAnswers != null). In other words, "if the call to clarification.getClarificationAnswers returns something other than null, execute the for loop which searches answers for a matching id".

However, in looking at the code in method Clarification.getClarificationAnswers(), it appears to me that it will ALWAYS return something other than null. I say this because the single line of code in that method is

return answerList.toArray(new ClarificationAnswer[answerList.size()]);

The variable answerList is declared and initialized with the following:

private ArrayList<ClarificationAnswer> answerList = new ArrayList<ClarificationAnswer>();

This means that answerList is an empty ArrayList to start with -- and that's what it'll still be if there no answers added to the answerList. Thus, answerList.toArray(new ClarificationAnswer[answerList.size()]); will always return a reference to an array (which will have size=0 if there are no answers). It will never return null.

Therefore, the test

clarAnswers = clarification.getClarificationAnswers(); if (clarAnswers != null) {

will always be true -- even if there are no answers. So the code enters the for loop looking for a matching answer -- but there are no answers.

It doesn't make sense to me why the code enters a loop to process answers if the condition is "there are no answers". That's what I was saying. It seems like the test if (clarAnswers != null) should actually be

clarAnswers = clarification.getClarificationAnswers(); if (clarAnswers != null && clarAnswers.length>0) {

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

clevengr · 2025-02-23T00:29:41Z

src/edu/csus/ecs/pc2/clics/API202306/ClarificationService.java

+            return Response.status(Status.BAD_REQUEST).entity("must include time property").build();
+        }
+
+        controller.getLog().log(Level.WARNING, "Admin PUT clarification is not implemented due to forcing the ID");


Well, I guess it still seems vague to me. Are you talking about the fact that PC2 can't support the ability of an external client to set the id of an existing clar?

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/EventFeedService.java. No major issues, although as I noted in one comment I'm very unclear on where the properties of an event are formally specified...

clevengr · 2025-02-23T06:12:00Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedService.java

+    public static String createEventFeedJSON(IInternalContest contest, IInternalController controller, HttpServletRequest servletRequest, SecurityContext sc) {
+        EventFeedStreamer streamer = new EventFeedStreamer(contest, controller, servletRequest, sc);
+        ByteArrayOutputStream stream = new ByteArrayOutputStream();
+        streamer.addStream(stream, new EventFeedFilter());


It would be nice if there was a comment noting that addStream() also outputs all past events to the stream (just to help the reader...) Otherwise, people (like me) will spend time trying to figure out why there are two consecutive statements

streamer.addStream(stream, new EventFeedFilter()); streamer.removeStream(stream);

Without knowing that side-effect of addStream(), that sequence makes little sense...

(And yes, I'm aware that this is legacy code...)

Agreed. Fixed in next push.

clevengr · 2025-02-23T06:29:53Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedService.java

+     * @return CLICSEndpoint object if the user can access this endpoint's properties, null otherwise
+     */
+    public static CLICSEndpoint getEndpointProperties(SecurityContext sc) {
+        String [] efProps = { "type", "id", "data", "token" };


It seems strange that these efProps are defined here as hard-coded strings; I would expect them to be defined as constants (somewhere). Also, I can't find any definition of these properties anywhere in the CLICS spec (some -- but not all -- of them appear in the Examples section under the Event feed endpoint description, but I can't find anywhere that they are actually defined/listed in the spec). (This of course is a problem with the spec, not with this code... but still I'm trying to figure out the source of their definition...)

I think I answered this in the slack chat, but I will duplicate that response here (without my editorial) for posterity:

The 2023-06 spec has it buried like other things in the spec. See "General Design Principles"->Notification Format. https://ccs-specs.icpc.io/2023-06/contest_api?highlight=num_solved#notification-format

I'm OK with having these as string literals in the array since they are specific to the protocol being implemented, and, are local to this module.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/EventFeedLog.java. One minor comment; no issues.

clevengr · 2025-02-23T06:34:50Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedLog.java

+import edu.csus.ecs.pc2.core.model.IInternalContest;
+
+/**
+ * Event fee log.


I didn't know there was a fee for the events... :)

(And yes, I know this is legacy code... ;)

I'm wondering if Doug was collecting a fee on the side for anyone who used the EF? :-)

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/EventFeedOperation.java. Other than the fact that this class isn't actually referenced anywhere in PC2 (and therefore maybe should be deleted), no problems.

clevengr · 2025-02-23T06:43:59Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedOperation.java

+ *
+ * @author Douglas A. Lane, PC^2 Team, [email protected]
+ */
+public enum EventFeedOperation {


It does not appear to me that this class (enum) is referenced anywhere in the PC2 project. Perhaps it should be deleted?

I have removed it. It is legacy. the "op" property is no longer part of the notification format, which is what this class was used for.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/EventFeedType.java. Left a couple of comments, some of which may be due to my lack of detailed understanding of how the Event Feed works...

clevengr · 2025-02-23T06:53:35Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedType.java

+    CONTESTS("contests"),
+    /**
+     *
+     */
+   CONTEST("contest"),


It's not clear to me why we have both a CONTEST and a CONTESTS type. In addition, I can't find any place in the CLICS spec that defines either of these event types (am I misreading the spec, or is this another of those places where the spec is, uh, "incomplete" ?)

These event types are for filtering on the event type field in a notification. Notification format description. Note that although the endpoint is called "/contests/" the notification type is "contest" in the EF sigh I kind of understand why... someone is asking about a specific contest by supplying the contest ID. Granted, that CONTESTS ("contests") should never be a type in the event feed for 2023-06, however, this is what it was called for 2020-03. I guess I carried it over and left it there since it wasn't harmful. Although, it may be misleading because we'll accept that as a valid type filter, but it will never allow any records to be sent on the feed since no notification should have a type of "contests". Removing it, would cause an IAE if someone supplied it. I think this may require a little more analysis to confirm what I just said, but I'm about 90% sure of it. I'll leave it for now.

clevengr · 2025-02-23T06:57:29Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedType.java

+    /**
+     *
+     */
+    AWARDS("awards"),


Shouldn't there be an ACCOUNTS type here (or somewhere in the enum)?

No. Accounts are not sent on the EF. See Notification format description

clevengr · 2025-02-23T06:59:46Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedType.java

+    /**
+     *
+     */
+    SUBMISSIONS("submissions"),


Shouldn't there be a SCOREBOARD type here (or somewhere in the enum)? Or do changes in the scoreboard not generate events?

No. Correct. See the spec notifications description as mentioned above.

clevengr

Reviewed src/edu/csus/ecs/pc2/clics/API202306/EventFeedStreamer.java. I left a couple of comments, but to be honest I am not completely sure how this class works (in conjunction with the related event fedd classes) so I don't really feel qualified to review it thoroughly...

clevengr · 2025-02-23T07:08:24Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedStreamer.java

+ *
+ * @author Douglas A. Lane, PC^2 Team, [email protected]
+ */
+public class EventFeedStreamer extends JSON202306Utilities implements Runnable, UIPlugin, IEventFeedStreamer {


It surprises me to see that this class implements... UIPlugin. I can't imagine where this class could plug in to a UI. (But maybe there's a reason. And yes, I realize it's been this way since, well, Doug :)

I'm guessing it may have had something to do with setContestAndController(), since UIPlugin does that. But, it seems any method that needs a contest or controller has it passed in. It may truly be legacy.

clevengr · 2025-02-23T07:32:05Z

src/edu/csus/ecs/pc2/clics/API202306/EventFeedStreamer.java

+
+    private JudgementListener judgementListener = new JudgementListener();
+
+    private ContestInformationListener contestInformationListener = new ContestInformationListener();


I see that the ContestInformationListener handles generating events for type CONTEST and for type STATE. I also see that there are listeners to handle events of type ACCOUNTS (teams), SUBMISSIONS (the RunListener), CLARIFICATIONS, PROBLEMS, LANGUAGES, GROUPS, and JUDGEMENTS. What's not clear to me is how events for JUDGEMENT_TYPES and ORGANIZATIONS are generated.

This may be just more of my lack of understanding of how the event feed works...

See JudgementListener in EventFeedStreamer around line 720. There is currently no way (that I am aware of) to add an organization to a contest that has been configured already. (IE no IOrganizationListener). Organizations are added during configuration only. Organizations events are only generated once when the EF is started and that's it. When organizations (institutions) are added the correct way and given their own model I suppose we'll add a listener for them then, but BFTF...

Various comment fixes and clarifications. Removed unused EventFeedOperation Class.

johnbrvc added CLICS CCS Update Changes to update to the CLICS specification Comments/Analysis Requested Soliciting comments and feedback from the Team about how to implement issue In PR code review Primary CCS Required for PC2 to act as primary CCS labels Nov 16, 2023

johnbrvc self-assigned this Nov 16, 2023

johnbrvc requested review from lane55, clevengr and troy2914 November 20, 2023 19:54

troy2914 reviewed Nov 23, 2023

View reviewed changes

cacerts.pc2 Outdated Show resolved Hide resolved

troy2914 requested changes Nov 23, 2023

View reviewed changes

pc2v9.ini Show resolved Hide resolved

troy2914 added this to the 9.10.0 milestone Nov 24, 2023

troy2914 reviewed Nov 25, 2023

View reviewed changes

src/edu/csus/ecs/pc2/clics/API202306/ScoreboardService.java Show resolved Hide resolved

troy2914 reviewed Nov 25, 2023

View reviewed changes

johnbrvc changed the base branch from v9.10 to develop November 30, 2023 21:37

johnbrvc mentioned this pull request Dec 14, 2023

Allow teams to be members of multiple groups #900

Closed

troy2914 reviewed Dec 15, 2023

View reviewed changes

src/edu/csus/ecs/pc2/clics/API202306/CLICSAccount.java Outdated Show resolved Hide resolved

troy2914 requested changes Dec 15, 2023

View reviewed changes

johnbrvc changed the title ~~I843 CLICS API 2023-06 Compliance~~ I843 CLICS API 2023-06 Endpoint Compliance Dec 25, 2023

troy2914 reviewed Jan 29, 2024

View reviewed changes

troy2914 modified the milestones: 9.10.0, 9.11.0 Feb 27, 2024

johnbrvc force-pushed the i843_CLICS_API_Endpoint_Compliance branch from 828ca5a to e0b0bec Compare December 10, 2024 16:06

clevengr removed the request for review from lane55 February 15, 2025 01:10

clevengr reviewed Feb 15, 2025

View reviewed changes

pc2v9.ini Show resolved Hide resolved

clevengr reviewed Feb 15, 2025

View reviewed changes

samps/pc2v9.ini Show resolved Hide resolved

i_843 PR Change requests

5ca0422

Added some comments and changed variable name to clarify freeze time calculation in CLICSContestState. Validate properties array for CLICSEndpoint.

clevengr reviewed Feb 20, 2025

View reviewed changes

src/edu/csus/ecs/pc2/clics/API202306/CLICSContestState.java Show resolved Hide resolved

clevengr reviewed Feb 20, 2025

View reviewed changes

clevengr requested changes Feb 20, 2025

View reviewed changes

pc2ccs deleted a comment from johnbrvc Feb 21, 2025

clevengr requested changes Feb 21, 2025

View reviewed changes

johnbrvc added 3 commits February 21, 2025 10:15

i_843 PR Change Requests

7312e58

Refactor CLICSRun -> CLICSJudgement, and all references. Various comment and log message clarifications. Fixed bug in CLICSProblemScore with returning the time as RELTIME instead of int (minutes).

i_843 i_1048 Fix test unit for correct operation of freeze duration

8fe4c17

Do not compare for leading 0 on freeze duration anymore.

clevengr reviewed Feb 22, 2025

View reviewed changes

clevengr requested changes Feb 22, 2025

View reviewed changes

i_843 PR change requests

f2fa54a

Mostly fixes to comments to correct misspellings. Some nomenclature changes for variables. Fixed CLICSVersionInfo class name spelling error.

clevengr reviewed Feb 22, 2025

View reviewed changes

src/edu/csus/ecs/pc2/clics/API202306/CLICSScore.java Show resolved Hide resolved

clevengr reviewed Feb 23, 2025

View reviewed changes

i_843 PR Change requests

1c2c126

Various comment fixes and clarifications. Removed unused EventFeedOperation Class.

		* CLICS Organization
		* Contains information about an Organization (institution)

		* @param account typical account using this organization
		* @param orgFields String array of fields that correspond to institutions.tsv. Gack!

		* CLICS Score (for a problem)
		* Contains information about the score a team received for a single problem

		* CLICS team score
		* Contains information the score for a team

		* CLICS Scoreboard
		* Contains information about the scoreboard

		* CLICS Scoreboard row
		* Contains information about a single entry in the scoreboard


		Clarification clar = null;

		// create list of clarifications to send back

		clarAnswers = clarification.getClarificationAnswers();
		if (clarAnswers != null) {


		private JudgementListener judgementListener = new JudgementListener();

		private ContestInformationListener contestInformationListener = new ContestInformationListener();

I843 CLICS API 2023-06 Endpoint Compliance #864

Are you sure you want to change the base?

I843 CLICS API 2023-06 Endpoint Compliance #864

Conversation

johnbrvc commented Nov 16, 2023 • edited Loading

Description of what the PR does

Issue which the PR addresses

Environment in which the PR was developed (OS,IDE, Java version, etc.)

Helpful information for reviewing the PR

Precise steps for testing the PR (i.e., how to demonstrate that it works correctly)

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 commented Dec 15, 2023

troy2914 left a comment

Choose a reason for hiding this comment

troy2914 commented Jan 29, 2024

johnbrvc commented Feb 3, 2024

clevengr left a comment

Choose a reason for hiding this comment

clevengr left a 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

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

clevengr left a 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

clevengr left a comment

Choose a reason for hiding this comment

clevengr Feb 20, 2025 • 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

clevengr left a 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

clevengr left a 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

johnbrvc Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

clevengr left a comment

johnbrvc commented Nov 16, 2023 •

edited

Loading

clevengr Feb 20, 2025 •

edited

Loading

johnbrvc Feb 21, 2025 •

edited

Loading