Skip to content
This repository was archived by the owner on Jun 23, 2022. It is now read-only.

First support for secondary metadata query. #114

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

First support for secondary metadata query. #114

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b99a471
First support for secondary metadata query.
aptls Dec 16, 2017
567df68
Code review changes.
Dec 19, 2017
40f13df
Code review changes.
Jan 4, 2018
6696f19
Code review changes.
Jan 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 113 additions & 12 deletions src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ private static boolean isNullOrEmptyString(String str) {
private String everyDocIdSql;
private String singleDocContentSql;
private MetadataColumns metadataColumns;
private String extraMetadataSql;
private MetadataColumns extraMetadataColumns;
private ResponseGenerator respGenerator;
private String aclSql;
private String aclPrincipalDelimiter;
Expand All @@ -150,6 +152,9 @@ public void initConfig(Config config) {
// when set to true, if "db.metadataColumns" is blank, it will use all
// returned columns as metadata.
config.addKey("db.includeAllColumnsAsMetadata", "false");
config.addKey("db.extraMetadataSql", "");
config.addKey("db.extraMetadataSqlParameters", "");
config.addKey("db.extraMetadataColumns", "");
config.addKey("db.modeOfOperation", null);
config.addKey("db.updateSql", "");
config.addKey("db.aclSql", "");
Expand Down Expand Up @@ -234,6 +239,27 @@ public void init(AdaptorContext context) throws Exception {
}
}

if (!cfg.getValue("db.extraMetadataSql").isEmpty()) {
extraMetadataSql = cfg.getValue("db.extraMetadataSql");
log.config("extra metadata sql: " + extraMetadataSql);
String extraMetadataColumnsConfig =
cfg.getValue("db.extraMetadataColumns");
if (extraMetadataColumnsConfig.isEmpty()) {
extraMetadataColumns = null;
log.config("extra metadata columns: Use all columns in ResultSet");
} else {
extraMetadataColumns = new MetadataColumns(extraMetadataColumnsConfig);
log.config("extra metadata columns: " + extraMetadataColumns);
}
} else {
if (!cfg.getValue("db.extraMetadataColumns").isEmpty()) {
ignored.add("db.extraMetadataColumns");
}
if (!cfg.getValue("db.extraMetadataSqlParameters").isEmpty()) {
ignored.add("db.extraMetadataSqlParameters");
}
}
Copy link
Contributor

@wiarlawd wiarlawd Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an else to check for non-empty db.extraMetadataColumns and db.extraMetadataSqlParameters and add them to ignored. You should add a test for that as well (I think ignoring both in one test is fine, or maybe even add it to an existing ignored property test; I'm not sure how those are organized.


modeOfOperation = cfg.getValue("db.modeOfOperation");
log.config("mode of operation: " + modeOfOperation);

Expand All @@ -258,6 +284,15 @@ public void init(AdaptorContext context) throws Exception {
} else if (!metadataColumns.isEmpty()) {
ignored.add("db.metadataColumns");
}
if (!cfg.getValue("db.extraMetadataSql").isEmpty()) {
ignored.add("db.extraMetadataSql");
}
if (!cfg.getValue("db.extraMetadataColumns").isEmpty()) {
ignored.add("db.extraMetadataColumns");
}
if (!cfg.getValue("db.extraMetadataSqlParameters").isEmpty()) {
ignored.add("db.extraMetadataSqlParameters");
}
}
} else if (singleDocContentSql.isEmpty()) {
throw new InvalidConfigurationException(
Expand Down Expand Up @@ -311,7 +346,9 @@ public void init(AdaptorContext context) throws Exception {
= new UniqueKey.Builder(cfg.getValue("db.uniqueKey"))
.setDocIdIsUrl(docIdIsUrl)
.setContentSqlColumns(cfg.getValue("db.singleDocContentSqlParameters"))
.setAclSqlColumns(cfg.getValue("db.aclSqlParameters"));
.setAclSqlColumns(cfg.getValue("db.aclSqlParameters"))
.setMetadataSqlColumns(
cfg.getValue("db.extraMetadataSqlParameters"));

// Verify all column names.
try (Connection conn = makeNewConnection()) {
Expand All @@ -332,6 +369,10 @@ public void init(AdaptorContext context) throws Exception {
"db.metadataColumns", metadataColumns.keySet());
}
}
if (extraMetadataColumns != null) {
verifyColumnNames(conn, "db.extraMetadataSql", extraMetadataSql,
"db.extraMetadataColumns", extraMetadataColumns.keySet());
}
if (respGenerator instanceof ResponseGenerator.SingleColumnContent) {
ResponseGenerator.SingleColumnContent content =
(ResponseGenerator.SingleColumnContent) respGenerator;
Expand Down Expand Up @@ -477,6 +518,9 @@ public void getDocIds(DocIdPusher pusher) throws IOException,
builder.setDeleteFromIndex(true);
} else if ("urlAndMetadataLister".equals(modeOfOperation)) {
addMetadataToRecordBuilder(builder, rs);
if (extraMetadataSql != null) {
addExtraMetadataToRecordBuilder(builder, conn, id.getUniqueId());
}
}
DocIdPusher.Record record = builder.build();
log.log(Level.FINEST, "doc id: {0}", id);
Expand All @@ -501,15 +545,10 @@ private interface MetadataHandler {
/*
* Adds all specified metadata columns to the record or response being built.
*/
private void addMetadata(MetadataHandler meta, ResultSet rs)
throws SQLException, IOException {
private void addMetadata(MetadataHandler meta, ResultSet rs,
MetadataColumns columns) throws SQLException, IOException {
ResultSetMetaData rsMetaData = rs.getMetaData();
synchronized (this) {
if (metadataColumns == null) {
metadataColumns = new MetadataColumns(rsMetaData);
}
}
for (Map.Entry<String, String> entry : metadataColumns.entrySet()) {
for (Map.Entry<String, String> entry : columns.entrySet()) {
int index;
try {
index = rs.findColumn(entry.getKey());
Expand Down Expand Up @@ -633,25 +672,73 @@ private void addMetadata(MetadataHandler meta, ResultSet rs)
@VisibleForTesting
void addMetadataToRecordBuilder(final DocIdPusher.Record.Builder builder,
ResultSet rs) throws SQLException, IOException {
synchronized (this) {
if (metadataColumns == null) {
metadataColumns = new MetadataColumns(rs.getMetaData());
}
}
addMetadata(
new MetadataHandler() {
@Override public void addMetadata(String k, String v) {
builder.addMetadata(k, v);
}
},
rs);
rs, metadataColumns);
}

@VisibleForTesting
void addMetadataToResponse(final Response resp, ResultSet rs)
throws SQLException, IOException {
synchronized (this) {
if (metadataColumns == null) {
metadataColumns = new MetadataColumns(rs.getMetaData());
Copy link
Contributor

@wiarlawd wiarlawd Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line isn't tested, since you lifted this out of the helper in two places, only the other line is executed.

}
}
addMetadata(
new MetadataHandler() {
@Override public void addMetadata(String k, String v) {
resp.addMetadata(k, v);
}
},
rs);
rs, metadataColumns);
}

private void addExtraMetadataToRecordBuilder(
final DocIdPusher.Record.Builder builder, Connection conn,
String uniqueId) throws SQLException, IOException {
MetadataHandler handler = new MetadataHandler() {
Copy link
Contributor

@wiarlawd wiarlawd Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe inline this as we do in addMetadataToXxx?

@Override public void addMetadata(String k, String v) {
builder.addMetadata(k, v);
}
};
addExtraMetadata(conn, uniqueId, handler);
}

private void addExtraMetadataToResponse(final Response resp, Connection conn,
String uniqueId) throws SQLException, IOException {
MetadataHandler handler = new MetadataHandler() {
@Override public void addMetadata(String k, String v) {
resp.addMetadata(k, v);
}
};
addExtraMetadata(conn, uniqueId, handler);
}

private void addExtraMetadata(Connection conn, String uniqueId,
MetadataHandler handler) throws SQLException, IOException {
try (PreparedStatement stmt = prepareExtraMetadataStatement(conn, uniqueId);
ResultSet rs = stmt.executeQuery()) {
synchronized (this) {
if (extraMetadataColumns == null) {
extraMetadataColumns = new MetadataColumns(rs.getMetaData());
}
}
while (rs.next()) {
addMetadata(handler, rs, extraMetadataColumns);
}
} catch (SQLException ex) {
throw new IOException(ex);
Copy link
Contributor

@wiarlawd wiarlawd Dec 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is untested. I'm not sure how to trigger this. A flat invalid query would fail in validation. Maybe a type mismatch in the parameter expression: <int-field> = ? with a character parameter? I can look harder if you can't find something easily.

}
}

/** Gives the bytes of a document referenced with id. */
Expand All @@ -675,6 +762,9 @@ public void getDocContent(Request req, Response resp) throws IOException {
}
// Generate response metadata first.
addMetadataToResponse(resp, rs);
if (extraMetadataSql != null) {
addExtraMetadataToResponse(resp, conn, id.getUniqueId());
}
// Generate Acl if aclSql is provided.
if (aclSql != null) {
resp.setAcl(getAcl(conn, id.getUniqueId()));
Expand Down Expand Up @@ -838,6 +928,14 @@ private PreparedStatement getStreamFromDb(Connection conn,
return st;
}

private PreparedStatement prepareExtraMetadataStatement(Connection conn,
String uniqueId) throws SQLException {
PreparedStatement st = conn.prepareStatement(extraMetadataSql);
uniqueKey.setMetadataSqlValues(st, uniqueId);
log.log(Level.FINER, "about to get extra metadata: {0}", uniqueId);
return st;
}

/**
* Mechanism that accepts stream of DocIdPusher.Record instances, buffers
* them, and sends them when it has accumulated maximum allowed amount per
Expand Down Expand Up @@ -921,7 +1019,7 @@ static ResponseGenerator loadResponseGenerator(Config config) {
throw new InvalidConfigurationException(errmsg, ex);
}
}

@VisibleForTesting
static ResponseGenerator loadResponseGeneratorInternal(Method method,
Map<String, String> config) {
Expand Down Expand Up @@ -1026,6 +1124,9 @@ public void getModifiedDocIds(DocIdPusher pusher)
builder.setDeleteFromIndex(true);
} else if ("urlAndMetadataLister".equals(modeOfOperation)) {
addMetadataToRecordBuilder(builder, rs);
if (extraMetadataSql != null) {
addExtraMetadataToRecordBuilder(builder, conn, id.getUniqueId());
}
}
DocIdPusher.Record record = builder.build();
log.log(Level.FINEST, "doc id: {0}", id);
Expand Down
57 changes: 43 additions & 14 deletions src/com/google/enterprise/adaptor/database/UniqueKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,17 @@ static enum ColumnType {
private final Map<String, ColumnType> types; // types of DocId columns
private final List<String> contentSqlCols; // columns for content query
private final List<String> aclSqlCols; // columns for acl query
private final List<String> metadataSqlCols; // columns for metadata query
private final boolean docIdIsUrl;

private UniqueKey(List<String> docIdSqlCols, Map<String, ColumnType> types,
List<String> contentSqlCols, List<String> aclSqlCols,
boolean docIdIsUrl) {
List<String> metadataSqlCols, boolean docIdIsUrl) {
this.docIdSqlCols = docIdSqlCols;
this.types = types;
this.contentSqlCols = contentSqlCols;
this.aclSqlCols = aclSqlCols;
this.metadataSqlCols = metadataSqlCols;
this.docIdIsUrl = docIdIsUrl;
}

Expand Down Expand Up @@ -114,18 +116,22 @@ String makeUniqueId(ResultSet rs) throws SQLException, URISyntaxException {

private void setSqlValues(PreparedStatement st, String uniqueId,
List<String> sqlCols) throws SQLException {
// parse on / that isn't preceded by escape char _
// (a / that is preceded by _ is part of column value)
String parts[] = uniqueId.split("(?<!_)/", -1);
if (parts.length != docIdSqlCols.size()) {
String errmsg = "Wrong number of values for primary key: "
+ "id: " + uniqueId + ", parts: " + Arrays.asList(parts);
throw new IllegalStateException(errmsg);
}
Map<String, String> zip = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
for (int i = 0; i < parts.length; i++) {
String columnValue = decodeSlashInData(parts[i]);
zip.put(docIdSqlCols.get(i), columnValue);
if (docIdIsUrl) {
zip.put(docIdSqlCols.get(0), uniqueId);
Copy link
Contributor

@wiarlawd wiarlawd Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. It feels so special case: if your doc ID is an URL (whether using urlAndMetadataLister mode or not), then your extraMetadataSql can only take that URL as a unique key parameter. We should make that explicit somewhere, maybe in a comment here. As we discussed, I think it would be preferable to try the route you suggested originally where the extraMetadataSqlParameters come from the singleDocContentSql result set, rather than from the doc ID. Maybe just a TODO here for now, I'm OK with exploring that in a separate change, but PJ may not be.

} else {
// parse on / that isn't preceded by escape char _
// (a / that is preceded by _ is part of column value)
String parts[] = uniqueId.split("(?<!_)/", -1);
if (parts.length != docIdSqlCols.size()) {
String errmsg = "Wrong number of values for primary key: "
+ "id: " + uniqueId + ", parts: " + Arrays.asList(parts);
throw new IllegalStateException(errmsg);
}
for (int i = 0; i < parts.length; i++) {
String columnValue = decodeSlashInData(parts[i]);
zip.put(docIdSqlCols.get(i), columnValue);
}
}
for (int i = 0; i < sqlCols.size(); i++) {
String colName = sqlCols.get(i);
Expand Down Expand Up @@ -171,6 +177,11 @@ void setAclSqlValues(PreparedStatement st, String uniqueId)
setSqlValues(st, uniqueId, aclSqlCols);
}

void setMetadataSqlValues(PreparedStatement st, String uniqueId)
throws SQLException {
setSqlValues(st, uniqueId, metadataSqlCols);
}

@VisibleForTesting
static String encodeSlashInData(String data) {
if (-1 == data.indexOf('/') && -1 == data.indexOf('_')) {
Expand Down Expand Up @@ -218,7 +229,7 @@ static String decodeSlashInData(String id) {
@Override
public String toString() {
return "UniqueKey(" + docIdSqlCols + "," + types + "," + contentSqlCols + ","
+ aclSqlCols + ")";
+ aclSqlCols + "," + metadataSqlCols + ")";
Copy link
Contributor

@wiarlawd wiarlawd Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Har, OK. Either way is fine.

}

/** Builder to create instances of {@code UniqueKey}. */
Expand All @@ -227,6 +238,7 @@ static class Builder {
private Map<String, ColumnType> types; // types of DocId columns
private List<String> contentSqlCols; // columns for content query
private List<String> aclSqlCols; // columns for acl query
private List<String> metadataSqlCols; // columns for metadata query
private boolean docIdIsUrl = false;

/**
Expand Down Expand Up @@ -345,6 +357,19 @@ Builder setAclSqlColumns(String aclSqlColumns)
return this;
}

Builder setMetadataSqlColumns(String metadataSqlColumns)
throws InvalidConfigurationException {
if (null == metadataSqlColumns) {
Copy link
Contributor

@wiarlawd wiarlawd Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Yoda conditions, we do not. (Used in the past, they have been, but no longer.)

throw new NullPointerException();
}
if (!metadataSqlColumns.trim().isEmpty()) {
this.metadataSqlCols =
splitIntoNameList("db.extraMetadataSqlParameters",
metadataSqlColumns, types.keySet());
}
return this;
}

private static List<String> splitIntoNameList(String paramConfig,
String cols, Set<String> validNames) {
List<String> columnNames = new ArrayList<String>();
Expand Down Expand Up @@ -434,6 +459,10 @@ List<String> getAclSqlColumns() {
return aclSqlCols;
}

List<String> getMetadataSqlColumns() {
return metadataSqlCols;
}

/** Return the Map of column types. */
@VisibleForTesting
Map<String, ColumnType> getColumnTypes() {
Expand Down Expand Up @@ -464,7 +493,7 @@ UniqueKey build() throws InvalidConfigurationException {
+ " The key must be a single string column when docId.isUrl=true.");
}
return new UniqueKey(docIdSqlCols, types, contentSqlCols, aclSqlCols,
docIdIsUrl);
metadataSqlCols, docIdIsUrl);
}
}
}
Loading