Skip to content

Passing the application's LoggerAdapter to requests #1565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 21, 2016
Merged

Passing the application's LoggerAdapter to requests #1565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 61 additions & 0 deletions spec/CloudCodeLogger.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
'use strict';
var LoggerController = require('../src/Controllers/LoggerController').LoggerController;
var FileLoggerAdapter = require('../src/Adapters/Logger/FileLoggerAdapter').FileLoggerAdapter;

describe("Cloud Code Logger", () => {

it("should expose log to functions", (done) => {
var logController = new LoggerController(new FileLoggerAdapter());

Parse.Cloud.define("loggerTest", (req, res) => {
req.log.info('logTest', 'info log', {info: 'some log' });
req.log.error('logTest','error log', {error: 'there was an error'});
res.success({});
});

Parse.Cloud.run('loggerTest').then(() => {
Parse.Cloud._removeHook('Functions', 'logTest');
return logController.getLogs({from: Date.now() - 500, size: 1000});
}).then((res) => {
expect(res.length).not.toBe(0);
let lastLogs = res.slice(0, 2);
let errorMessage = lastLogs[0];
let infoMessage = lastLogs[1];
expect(errorMessage.level).toBe('error');
expect(errorMessage.error).toBe('there was an error');
expect(errorMessage.message).toBe('logTest error log');
expect(infoMessage.level).toBe('info');
expect(infoMessage.info).toBe('some log');
expect(infoMessage.message).toBe('logTest info log');
done();
});
});

it("should expose log to trigger", (done) => {
var logController = new LoggerController(new FileLoggerAdapter());

Parse.Cloud.beforeSave("MyObject", (req, res) => {
req.log.info('beforeSave MyObject', 'info log', {info: 'some log' });
req.log.error('beforeSave MyObject','error log', {error: 'there was an error'});
res.success({});
});

let obj = new Parse.Object('MyObject');
obj.save().then(() => {
Parse.Cloud._removeHook('Triggers', 'beforeSave', 'MyObject');
return logController.getLogs({from: Date.now() - 500, size: 1000})
}).then((res) => {
expect(res.length).not.toBe(0);
let lastLogs = res.slice(0, 2);
let errorMessage = lastLogs[0];
let infoMessage = lastLogs[1];
expect(errorMessage.level).toBe('error');
expect(errorMessage.error).toBe('there was an error');
expect(errorMessage.message).toBe('beforeSave MyObject error log');
expect(infoMessage.level).toBe('info');
expect(infoMessage.info).toBe('some log');
expect(infoMessage.message).toBe('beforeSave MyObject info log');
done();
});
});
});
6 changes: 4 additions & 2 deletions spec/FileLoggerAdapter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ describe('info logs', () => {
var fileLoggerAdapter = new FileLoggerAdapter();
fileLoggerAdapter.info('testing info logs', () => {
fileLoggerAdapter.query({
size: 1,
from: new Date(Date.now() - 500),
size: 100,
level: 'info'
}, (results) => {
if(results.length == 0) {
Expand All @@ -28,7 +29,8 @@ describe('error logs', () => {
var fileLoggerAdapter = new FileLoggerAdapter();
fileLoggerAdapter.error('testing error logs', () => {
fileLoggerAdapter.query({
size: 1,
from: new Date(Date.now() - 500),
size: 100,
level: 'error'
}, (results) => {
if(results.length == 0) {
Expand Down
4 changes: 2 additions & 2 deletions src/RestWrite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ RestWrite.prototype.runBeforeTrigger = function() {
updatedObject.set(this.sanitizedData());

return Promise.resolve().then(() => {
return triggers.maybeRunTrigger(triggers.Types.beforeSave, this.auth, updatedObject, originalObject, this.config.applicationId);
return triggers.maybeRunTrigger(triggers.Types.beforeSave, this.auth, updatedObject, originalObject, this.config);
}).then((response) => {
if (response && response.object) {
this.data = response.object;
Expand Down Expand Up @@ -821,7 +821,7 @@ RestWrite.prototype.runAfterTrigger = function() {
this.config.liveQueryController.onAfterSave(updatedObject.className, updatedObject, originalObject);

// Run afterSave trigger
triggers.maybeRunTrigger(triggers.Types.afterSave, this.auth, updatedObject, originalObject, this.config.applicationId);
triggers.maybeRunTrigger(triggers.Types.afterSave, this.auth, updatedObject, originalObject, this.config);
};

// A helper to figure out what location this operation happens at.
Expand Down
10 changes: 5 additions & 5 deletions src/Routers/FunctionsRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ var express = require('express'),
import PromiseRouter from '../PromiseRouter';

export class FunctionsRouter extends PromiseRouter {

mountRoutes() {
this.route('POST', '/functions/:functionName', FunctionsRouter.handleCloudFunction);
}

static createResponseObject(resolve, reject) {
return {
success: function(result) {
Expand All @@ -26,19 +26,19 @@ export class FunctionsRouter extends PromiseRouter {
}
}
}

static handleCloudFunction(req) {
var applicationId = req.config.applicationId;
var theFunction = triggers.getFunction(req.params.functionName, applicationId);
var theValidator = triggers.getValidator(req.params.functionName, applicationId);
if (theFunction) {

const params = Object.assign({}, req.body, req.query);
var request = {
params: params,
master: req.auth && req.auth.isMaster,
user: req.auth && req.auth.user,
installationId: req.info.installationId
installationId: req.info.installationId,
log: req.config.loggerController && req.config.loggerController.adapter
Copy link
Contributor

Choose a reason for hiding this comment

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

You think we should expose the loggerController so we can perform additional tasks if needed instead of forwarding directly the 'unsafe' adapter. We try, as much as we can to avoid exposing the adapters directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think that in this case exposing the adapter directly makes sense. That gives LoggerAdapter implementers the ability to be innovative, without imposing a high implementation burden on them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've thought about this a bit more and I have thought of a good reason for LoggerAdapter to have a consistent interface: If any other adapter (such as DB adapter) want to log stuff, we can give them a logger so that they can do that. Perhaps we should rethink this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@flovilmart

Copy link
Contributor

Choose a reason for hiding this comment

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

usually logger interfaces are pretty standard for writing (.log, .info, .error etc...) however for reading it's another issue....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading is quite interesting, a lot of loggers i have looked at don't support reading at all.

For people wanting to use their own loggers, I'm not sure how important having the read call is.

I can see it being very useful for people who don't care for configuring their own logger though, so they can quickly check their logs in the dashboard (However that's currently broken and no one has said anything more in the bug i made about it #1563 ).

I would just leave the logger adapter where it is now and see if anyone asks for more.
I personally would like a .tracebut 90% of my log calls in cloud code areinfoorerror``

Copy link
Contributor

@flovilmart flovilmart Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't had the time to look at #1563 and if that's an issue in a dependency, we should open it there, and submit a PR to it :)

The more it goes, the more I believe we could pass a logger object as an option (as the AWS node SDK does).

Then the loggerAdapter could have it's functionality reserved reading logs, this way a bunyan logger that don't provide reading would not be bothered with the current adapter.

};

if (theValidator && typeof theValidator === "function") {
Expand Down
4 changes: 2 additions & 2 deletions src/rest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ function del(config, auth, className, objectId) {
inflatedObject = Parse.Object.fromJSON(response.results[0]);
// Notify LiveQuery server if possible
config.liveQueryController.onAfterDelete(inflatedObject.className, inflatedObject);
return triggers.maybeRunTrigger(triggers.Types.beforeDelete, auth, inflatedObject, null, config.applicationId);
return triggers.maybeRunTrigger(triggers.Types.beforeDelete, auth, inflatedObject, null, config);
}
throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND,
'Object not found for delete.');
Expand All @@ -79,7 +79,7 @@ function del(config, auth, className, objectId) {
objectId: objectId
}, options);
}).then(() => {
triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config.applicationId);
triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config);
return Promise.resolve();
});
}
Expand Down
23 changes: 13 additions & 10 deletions src/triggers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const baseStore = function() {
base[key] = {};
return base;
}, {});

return Object.freeze({
Functions,
Validators,
Expand Down Expand Up @@ -63,7 +63,7 @@ export function getTrigger(className, triggerType, applicationId) {
throw "Missing ApplicationID";
}
var manager = _triggerStore[applicationId]
if (manager
if (manager
&& manager.Triggers
&& manager.Triggers[triggerType]
&& manager.Triggers[triggerType][className]) {
Expand Down Expand Up @@ -92,15 +92,18 @@ export function getValidator(functionName, applicationId) {
return undefined;
}

export function getRequestObject(triggerType, auth, parseObject, originalParseObject) {
export function getRequestObject(triggerType, auth, parseObject, originalParseObject, config) {
var request = {
triggerName: triggerType,
object: parseObject,
master: false
master: false,
log: config.loggerController && config.loggerController.adapter
};

if (originalParseObject) {
request.original = originalParseObject;
}

if (!auth) {
return request;
}
Expand Down Expand Up @@ -145,19 +148,19 @@ export function getResponseObject(request, resolve, reject) {
// Resolves to an object, empty or containing an object key. A beforeSave
// trigger will set the object key to the rest format object to save.
// originalParseObject is optional, we only need that for befote/afterSave functions
export function maybeRunTrigger(triggerType, auth, parseObject, originalParseObject, applicationId) {
export function maybeRunTrigger(triggerType, auth, parseObject, originalParseObject, config) {
if (!parseObject) {
return Promise.resolve({});
}
return new Promise(function (resolve, reject) {
var trigger = getTrigger(parseObject.className, triggerType, applicationId);
var trigger = getTrigger(parseObject.className, triggerType, config.applicationId);
if (!trigger) return resolve();
var request = getRequestObject(triggerType, auth, parseObject, originalParseObject);
var request = getRequestObject(triggerType, auth, parseObject, originalParseObject, config);
var response = getResponseObject(request, resolve, reject);
// Force the current Parse app before the trigger
Parse.applicationId = applicationId;
Parse.javascriptKey = cache.apps.get(applicationId).javascriptKey || '';
Parse.masterKey = cache.apps.get(applicationId).masterKey;
Parse.applicationId = config.applicationId;
Parse.javascriptKey = config.javascriptKey || '';
Parse.masterKey = config.masterKey;
trigger(request, response);
});
};
Expand Down