[GitHub] [trafficcontrol] moltzaum opened a new pull request #3484: TO: "info" struct tag example for better generic endpoint handling

[GitBox <gi...@apache.org>]

moltzaum opened a new pull request #3484: TO: "info" struct tag example for better generic endpoint handling
URL: https://github.com/apache/trafficcontrol/pull/3484
 
 
   ## What does this PR do?
   
   This PR is related to the joined-fields issue: #2494
   
   The issue of joined-fields was also briefly mentioned in #3107, where it is said that "ideally this would be fixable in the framework rather than requiring changes on a per-handler basis".
   
   This PR implements joined fields for ASNs in a generic fashion. The reason I have only implemented the ASN handler is because replacing the current generic implementation with the one I did requires a change in struct tags for every endpoint it affects. I don't mind doing this, but I want to more or less present what I have now for potential feedback.
   
   **How would the struct tags change?**
   
   ```
   type ASNNullable struct {
   	ASN          *int       `json:"asn"          db:"asn"`
   	Cachegroup   *string    `json:"cachegroup"   db:"cachegroup"`
   	CachegroupID *int       `json:"cachegroupId" db:"cachegroup_id"`
   	ID           *int       `json:"id"           db:"id"`
   	LastUpdated  *TimeNoMod `json:"lastUpdated"  db:"last_updated"`
   }
   ```
   ```
   type ASNNullable struct {
   	ASN          *int       `json:"asn"`
   	Cachegroup   *string    `json:"cachegroup"   info:"join"`
   	CachegroupID *int       `json:"cachegroupId"`
   	ID           *int       `json:"id"           info:"key"`
   	LastUpdated  *TimeNoMod `json:"lastUpdated"  info:"lastUpdated"`
   }
   ```
   
   **What does the info tag do?**
   
   It allows us to say `fields["join"]` and get all fields in the struct that have the info struct tag of "join".
   Any value may be used, and for now I just specify "join", "key", and "lastUpdated".
   
   **What advantage do we have with info tags over db tags?**
   
   db struct tags are used with `NamedQuery`, `NamedExec`, and `StructScan`. In sql queries we may use syntax of the form `:field` to reference a field with `db:"field"`. The db tags use reflection to find the fields.
   
   For a single endpoint implementation there is no need to use a named query. I believe _some_ endpoints have db tags because of copy-pasta, then used the `StructScan` for convenience. However, the main user of the named exec are the generic methods that greatly benefit from the reflection because the cruder only gets to work with interfaces.
   
   The info tag gives us more control over the result of the reflection. Instead of just using the result to replace a name for a value in query, we can also use it to scan items into, since the sql database functions use interfaces (holding pointers of any type).
   
   It is relevant to note that the named functions we are currently using is from `sqlx`. If we no longer use these functions anymore there is potential to remove a dependency that we don't need.
   
   **What endpoints could this affect**
   
   1) All endpoints in `asns`, `deliveryservices`, `regions`, `phys_locations`, `servers`, `tenants` from #2494  
   2) Endpoints that did not use the generic methods in order to include join fields properly (I'm not sure specifically what endpoints). That being said, where tenancy is needed there is still no generic method created yet (methods exist, but the `GenericCreate`, `GenericUpdate`, etc. does not know how to check tenancy).
   
   **Additional benefits beyond just replacing the db tag**
   
   In our code we have methods like `SetLastUpdated`, `GetKeyFieldInfo`, etc. `SetLastUpdated` is no longer needed because with the info tag, we have recieved the pointer and scan into it. Some instances of `SetKeys` will also be removed for the same reason. I think in the future it would be beneficial to consider the complete removal of a struct's implementation for how it handles keys since this can simplify the logic and reduce boilerplate.
   
   ## Which Traffic Control components are affected by this PR?
   
   - Traffic Ops
   
   ## If this is a bug fix, what versions of Traffic Ops are affected?
   <!-- If this PR fixes a bug, please list here all of the affected versions - to
   the best of your knowledge. It's also pretty helpful to include a commit hash
   of where 'master' is at the time this PR is opened (if it affects master),
   because what 'master' means will change over time. For example, if this PR
   fixes a bug that's present in master (at commit hash '2697ebac'), in v3.0.0,
   and in the current 3.0.1 Release candidate (e.g. RC1), then this list would
   look like:
   
   - master (2697ebac)
   - 3.0.0
   - 3.0.1 (RC1)
   
   If you don't know what other versions might have this bug, AND don't know how
   to find the commit hash of 'master', then feel free to leave this section
   blank (or, preferably, delete it entirely).
    -->
   This bug has been persistent since the affected endpoints were rewritten in go.
   
   ## The following criteria are ALL met by this PR
   
   Although there are some tests, I haven't tested odd cases yet.
   The documentation I've included is GoDoc
   
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
     - I'm not _fully_ sure when a changelog is necessary.
     - These changes are mostly internal, but also affect the visibility of certain joined fields.
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services