Conversation
| import java.util.Collections; | ||
| import javax.sql.DataSource; | ||
|
|
||
| public class InitYoucatTS extends InitDatabaseTS { |
There was a problem hiding this comment.
This tight coupling is not going to actually work. There are both create and update sets of SQL and this looks plausible for create (although subtle). But for update it's never going to be viable to extend the base when there are conceptually two separate things.
As long as this is a youcat only feature, it has to have an independent InitDatabase extension with it's own previous and current version.
There was a problem hiding this comment.
Created a InitDatabaseYoucat class for the ServiceDescriptor table.
|
|
||
| private String getOwnerID(Subject owner) { | ||
| IdentityManager identityManager = AuthenticationUtil.getIdentityManager(); | ||
| return (String) identityManager.toOwner(owner); |
There was a problem hiding this comment.
This cast is wrong and can fail. IdentityManager.toOwner(Subject) returns Object, not String, and does not always return String. It's true that the reverse IdentityManager.toSubject(Object) should be able to reconstruct from a String, but toSubject has to do more anyway. toOwner could return a Principal or more primitive value or a String... not ideal and we could think about restricting the types from Object, but that requires some thought.
To support different types, you need to have code to convert the Object to a String to store it as a String
There was a problem hiding this comment.
Not converting the ownerID to a string property, and checking it was null, was hiding a bug in the intTest, which isn't configuring a IdentityManager, hence getting the ownerID from an owner Subject returns null from the IdentityManager.
|
|
||
| // generate the key for the KeyValue pair | ||
| private String generateKey(String name, Object ownerID) { | ||
| return name + ":" + ownerID; |
There was a problem hiding this comment.
implicitly using ownerID.toString() here, which might not be right
| import ca.nrc.cadc.db.version.KeyValue; | ||
| import org.apache.log4j.Logger; | ||
|
|
||
| public class ServiceDescriptors extends KeyValue { |
There was a problem hiding this comment.
The KeyValueDAO constructor takes a class and uses the class name as the name of the KeyValue table.
There was a problem hiding this comment.
sure, but in this case the table stores ServiceDescritorTemplate(s), so you don't need to make up a new class just to name the table. There might be an argument to think about how we'll transition to a custom table in future and what that might be called... for now, just use the ServiceDescritorTemplate.class
aside: recall that in caom2 an Observation is stored in the Observation table, not Observations, uws.Job table, etc. Ignore the naming style in tap_schema core (with plurals)... that's a bad way to go.
There was a problem hiding this comment.
also the initial use case for KeyVaueDAO: ModelVersion, non-plural
| import java.util.Collections; | ||
| import javax.sql.DataSource; | ||
|
|
||
| public class InitDatabaseYoucat extends InitDatabase { |
There was a problem hiding this comment.
This is probably poor name because it implies a complete solution... how about InitDatabaseSD (for ServiceDescriptor, which is the core concept). Then this could be an optional feature and concept and config naming would match the code.
There was a problem hiding this comment.
Agree and updated.
2 participants
A draft since it depends on cadc-tap-schema 1.28 which is not yet published and will not build.
I didn't add intTest's for the DAO list methods, wanted to see if IdentityManager usage is correct in the DAO before trying to find a way to use an IdentityManager in the intTest such that doesn't want to augment a Subject.